Follow-up to #2880013: Add command to check if .es6.js has been transpiled

Problem/Motivation

When transpiling JS files we sensibly add a comment to thew top of each file which tells people not to change the file and which file to change.

Unfortunately,

yarn run build:js -- --file ./misc/active-link.es6.js

and

yarn run build:js -- --file misc/active-link.es6.js

and

yarn run build:js -- --file ~/dev/sites/drupal8alt.dev/core/misc/active-link.es6.js

The latter results in the current a diff like:

diff --git a/core/misc/active-link.js b/core/misc/active-link.js
index b9700a1..4fb364d 100644
--- a/core/misc/active-link.js
+++ b/core/misc/active-link.js
@@ -1,6 +1,6 @@
 /**
 * DO NOT EDIT THIS FILE.
-* All changes should be applied to ./misc/active-link.es6.js
+* All changes should be applied to /Users/alex/dev/sites/drupal8alt.dev/core/misc/active-link.es6.js
 * See the following change record for more information,
 * https://www.drupal.org/node/2873849
 * @preserve

Proposed resolution

Remove file path. It is not that helpful. Also https://www.drupal.org/node/2873849 is not the correct CR - https://www.drupal.org/node/2815083 is.

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#10 2880560-9.patch72.35 KBalexpott
#8 c20170523_180727.png23.27 KBdroplet
#2 2880560-2.patch77.43 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
Issue tags: +JavaScript
FileSize
2880560-2.patch77.43 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
+++ b/core/scripts/js/compile.js
@@ -1,6 +1,10 @@
 const babel = require('babel-core');
+const fs = require('fs');
+const path = require('path');
 
 module.exports = (filePath, callback) => {
+  // Make the path relative to DRUPAL_ROOT and standardise it.
+  let commentFilePath = path.relative(fs.realpathSync(__dirname + "/../../../"), path.resolve(fs.realpathSync(filePath)));
   // Transform the file.
   // Check process.env.NODE_ENV to see if we should create sourcemaps.
   babel.transformFile(
@@ -11,7 +15,7 @@ module.exports = (filePath, callback) => {

@@ -11,7 +15,7 @@ module.exports = (filePath, callback) => {
       plugins: [
         ['add-header-comment', {
           'header': [
-            `DO NOT EDIT THIS FILE.\nAll changes should be applied to ${filePath}\nSee the following change record for more information,\nhttps://www.drupal.org/node/2873849\n@preserve`
+            `DO NOT EDIT THIS FILE.\nAll changes should be applied to ${commentFilePath}\nSee the following change record for more information,\nhttps://www.drupal.org/node/2873849\n@preserve`
           ]
         }]
       ]

Here's the important change. The rest of the patch is just compiled JS changing.

droplet’s picture

droplet CreditAttribution: droplet commented

I think we can remove this line.

First, our ES6 always next to non-ES6.
Second, if you're the first time to contribute. It's worth to read #2873849 first. To understand the whole build process. (You're not just applying changes to ES6, but has build process)
Third, gzipped, smaller file size, hehe

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

@droplet the line is there to help people learn - you know how things are working so it seems obvious and redundant. But this line is pointing to #2873849 so it just helps.

droplet’s picture

droplet CreditAttribution: droplet commented

@alexpott,

No. If you understand nothing, you read the sentences and edit ES6 and then `git diff`. Boom!

BUT, if we didn't provide anything there, you may guess or should read #2873849 to understand.

We have no `src` and `build` dir. So we need not provide a full path to help to discover the file.

It's better linking to https://www.drupal.org/node/2815083 instead

Ideally, we should create a new Docs. We need not explain background story that why we did it. Just instructions to patch.

droplet’s picture

droplet CreditAttribution: droplet commented

Just keep this:

`DO NOT EDIT THIS FILE.\n See the following change record for more information,\nhttps://www.drupal.org/node/2815083\n@preserve`

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
c20170523_180727.png23.27 KB

even better (SPACE won't increasing file sizes a lot):

I know it's a bit crazy :p
As experienced developer, I won't start from LINE1 (won't start from comments, usually it collapsed auto)

droplet’s picture

droplet CreditAttribution: droplet commented
Related issues: +#2880597: Removing the ES6 comment at the top of .js when "Aggregate JavaScript files" enabled
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2880560-9.patch72.35 KB

@droplet ok I think you are right - including the corresponding file is not that helpful because in order to know what to edit and then what commands to run you should read https://www.drupal.org/node/2815083 (Nice catch)

So let's remove it and swap the CR link.

No interdiff because it would be twice the size of the patch file.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Title: Ensure that comment added to top of .js files is command independent » Fix comment added to top of .js files by build process
Issue summary: View changes 
+++ b/core/scripts/js/compile.js
@@ -11,7 +11,7 @@ module.exports = (filePath, callback) => {
       plugins: [
         ['add-header-comment', {
           'header': [
-            `DO NOT EDIT THIS FILE.\nAll changes should be applied to ${filePath}\nSee the following change record for more information,\nhttps://www.drupal.org/node/2873849\n@preserve`
+            `DO NOT EDIT THIS FILE.\nSee the following change record for more information,\nhttps://www.drupal.org/node/2815083\n@preserve`
           ]
         }]
       ]

Here's the only non-automated change in this patch.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Reviewed & tested by the community

Go Go Go!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Adding the filepath to the comment was @drpal's and my idea in #2818825-10: Rename all JS files to *.es6.js and compile them - we got it wrong. But adding the comment was a good idea.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

Alright. I think this is actually much better. +1 for this :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

  • catch committed f3cf9c4 on 8.4.x 
    Issue #2880560 by alexpott, droplet: Fix comment added to top of .js...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.