Comments

nod_ created an issue. See original summary.

nod_’s picture

big rename & compile. Should probably wait to have a good change record before moving on this.

nod_’s picture

Status: Active » Needs review
FileSize
711 bytes
3.31 MB

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drpal’s picture

This patch duplicates all JavaScript files in {misc,modules,themes}, replacing their extension with, .es6.js.

This should be committed before #2815077: Adopt airbnb javascript style guide v13 as new baseline javascript coding standards for Drupal 8 core and contrib, which will add ES6 build tooling enabling us to effectively ignore *.js files.

drpal’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 5: 2818825-5.patch, failed testing.

drpal’s picture

Status: Needs work » Needs review
drpal’s picture

This is exactly currently as of 6154128112.

drpal’s picture

After talking with @alexpott I think it makes sense to include a 'built' copy of each of these files. Additionally a comment is added to the top of the built copy indicating to not edit the built copy.

nod_’s picture

Status: Needs review » Needs work

If we're not minifing, we need to get rid of comments in the transpiled files at least. Adding @preserve to the comment header will tell babel to keep the comment around.

      comments: false,
      plugins: [
        ['add-header-comment', {
          'header': [
            `DO NOT EDIT THIS FILE. \nAll changes should be applied to the ${filePath} variant.\n@preserve`
          ]
        }]
      ]

Side note, the lint:core-js shouldn't auto fix issues. testbot won't complain but it won't be right either way.
The rest looks fine I guess. as long as the copy script was used we shouldn't have problems.

drpal’s picture

Status: Needs work » Needs review
FileSize
1.96 MB

Alright, comment removal is back! Additionally the header note is being kept, thanks @nod_. Lets create a followup to remove the --fix part of linting?

nod_’s picture

Everything looks good to me.

We'll get another huge round of code standard fix :p

Not sure who can RTBC this though :p

drpal’s picture

Assigned: Unassigned » droplet

@droplet

Can you have a look at this? Thanks.

droplet’s picture

Assigned: droplet » drpal
Status: Needs review » Needs work

My Review Process:

# always grab online file
curl https://www.drupal.org/files/issues/2818825-12.patch | git apply --index

# yarn it
yarn

# build it
npm run build:js

# any diff?
git status
git status:
modified: modules/node/content_types.js

==========

    "build:js": "node ./scripts/js/babel-es6-build.js",

Should we add cross-env NODE_ENV=production to build:js ?

drpal’s picture

Assigned: drpal » droplet
Status: Needs work » Needs review

@droplet

The changes to content_types.js is expected since this patch contains the original JavaScript passed through the transpiler, which at this point only removes the comments, leaving a note to edit only the .es6.js file.

We don't have any specific build steps that would require a production flag. Perhaps if we decide to minify the output later we can add that?

droplet’s picture

Assigned: droplet » drpal

@drpal

this is a recent change:
https://www.drupal.org/node/2849100#comment-12084807

We don't have any specific build steps that would require a production flag. Perhaps if we decide to minify the output later we can add that?

Cool. No more default to development:
https://github.com/babel/babel/pull/4924/files
Misread. It still default to `development` and deprecating env in 7.0: https://github.com/babel/babel/issues/5276

Any other better way to educate our developers to use this command:

// yarn run build:js -- --files misc/drupal.es6.js misc/drupal.init.es6.js
droplet’s picture

Status: Needs review » Needs work

Also,

please create Git diff with -C flag.

git diff -C

Thanks

drpal’s picture

Assigned: drpal » droplet
Status: Needs work » Needs review
FileSize
1.05 MB

@droplet

We still want to specify the environment since that dictates if we are going to generate sourcemaps or not. I'll update the change record to include the specifics around using the --files flag for building. And a new patch with -C

drpal’s picture

Change record updated with some documentation about building only a single file, [#2873849].

droplet’s picture

Assigned: droplet » Unassigned
Status: Needs review » Needs work

#19 still using old version of modules/node/content_types.js

droplet’s picture

drpal’s picture

@droplet

#19 still using old version of modules/node/content_types.js

What do you mean?

droplet’s picture

Patch contains the outdated version

diff --git a/core/modules/node/content_types.js b/core/modules/node/content_types.es6.js
similarity index 95%
copy from core/modules/node/content_types.js
copy to core/modules/node/content_types.es6.js
index 4ec34aa05e..eed93f01c9 100644
--- a/core/modules/node/content_types.js
+++ b/core/modules/node/content_types.es6.js
@@ -26,7 +26,7 @@
       });
       $context.find('#edit-workflow').drupalSetSummary(function (context) {
         var vals = [];
-        $(context).find('input[name^="options"]:checked').next('label').each(function () {
+        $(context).find('input[name^="options"]:checked').parent().each(function () {
           vals.push(Drupal.checkPlain($(this).text()));
         });
         if (!$(context).find('#edit-options-status').is(':checked')) {
drpal’s picture

@droplet

Thanks, I'll re-roll against the newest head.

droplet’s picture

git diff --cached -C --stat | grep es6.js

this command should output ZERO changes.

drpal’s picture

Status: Needs work » Needs review
FileSize
1.03 MB

Alright, this was created as of 5184e99d3f.

droplet’s picture

Missing the banner. otherwise, it looks good. Thanks!

drpal’s picture

@droplet

My fault, I forgot that the header changes were not included till now. This is still current with 5184e99d3f.

droplet’s picture

drpal’s picture

@droplet

First try.

droplet’s picture

Status: Needs review » Reviewed & tested by the community

Go Go GO!

drpal’s picture

U+1F64F

nod_’s picture

RTBC +1

nod_’s picture

Issue tags: +frontend performance

For information a side-effect of removing comments removes about 150KB–200KB of JS on all pages. I checked with minification, it helps a little but not as much.

cilefen’s picture

Could the “DO NOT EDIT THIS FILE.” comment link to the CR?

nit: there is an annoying space after "DO NOT EDIT THIS FILE"

I am basically a +1 for this and I like the enthusiasm. It's difficult to make sure code reflows didn't break something but I guess that is far-fetched. We'll have to open a PR on https://github.com/alexpott/d8githooks to eslint the es6 files going forward.

Dinesh18’s picture

works as expected.

drpal’s picture

A minor update to the comment at the top of the built JavaScript files to include a link to the change record.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work
drpal’s picture

Status: Needs work » Needs review
FileSize
1.98 MB

Rerolled as of 01621e5880.

drpal’s picture

My fault, missed the -C flag when creating the patch. Exactly the same as the patch in #40.

droplet’s picture

Status: Needs review » Reviewed & tested by the community

Hmmm.. #2831274: Bring Media entity module to core as Media module re-introduced a common JS bug (and bad code style) into CORE I think..

RTBC again. Other wording, banner things we able to fix in a follow-up. Let's get rid of a huge patch first :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8287017 and pushed to 8.4.x. Thanks!

  • alexpott committed 8287017 on 8.4.x
    Issue #2818825 by drpal, nod_, droplet, cilefen: Rename all JS files to...

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture