Follow-up to #2815077: Adopt airbnb javascript style guide v13 as new baseline javascript coding standards for Drupal 8 core and contrib

Problem/Motivation

Our current build process for ES6 is lacking some features, and is already out of date for transpile settings.

Proposed resolution

Move away from babel-preset-es2015 to babel-preset-env. Allowing us to specify targets for our built code, making it easier to only transpile the parts of our code that are incompatible with the browser environments we are currently supporting.

Remaining tasks

This patch should be applied, and will probably need a reroll, after #2815077: Adopt airbnb javascript style guide v13 as new baseline javascript coding standards for Drupal 8 core and contrib is finalized.

Comments

drpal created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, rollup.patch, failed testing.

drpal’s picture

Status: Needs work » Needs review
droplet’s picture

Thanks @drpal, I waited for other comments for few day. Unfortunately no comments. Here's my little thoughts.

wrap transpiling with Rollup, module bundler for JavaScript, allowing us to use polyfills for features such as Object.entries.

@drpal, can you explain this point? Thanks!

** If we decided to go with Rollup. I think we can do it wiser and use Rollup's config / watcher.

There are few things I think we need to think 10000x times before introducing new toolings:

1. Adding new things in Drupal is hard but removing them would be 10000000000000000000x harder.
(Do it later, when we have a clear direction on how to organize the JS will be better than now)

2. Compile Performance. D8's JS are very lightweight. We have no HMR.

drpal’s picture

@droplet

Sure.

wrap transpiling with Rollup, module bundler for JavaScript, allowing us to use polyfills for features such as Object.entries.

Take Object.entries() as an example. If we want to use this, pretty useful for looking at objects, Babel will do nothing with it since it's just a syntax transpiler. If you want to include a polyfill for Object.entries() then we need a way to get it into the browser environment. We could just load a polyfill library on every page, probably not a good idea, or include a portion of core-js with a require/import statement and have those polyfills bundled with the files that need them.

There are some issues around that I can see, including a polyfill more than once, or forgetting to include it and breaking out of date browsers.

If we decided to go with Rollup. I think we can do it wiser and use Rollup's config / watcher.

Sure, whatever technical implementation we decide on is probably alright. I was looking to make the least amount of changes here and continue to use the existing watchers and JS api.

Compile Performance. D8's JS are very lightweight. We have no HMR

I've migrated about half of core/misc at this point. Compile performance has never been an issue. Additionally with Babili the built files are incredibly small and can still utilize the source maps.

drpal’s picture

FileSize
5.68 KB

I've flipped back and forth on this a few times. At this point I'm not so sold on having a bundler like Rollup or Webpack. Both add a significant overhead and don't really work with how our JavaScript is developed and deployed into individual files.

This patch rolls that back and keeps just the updates to babel-present-env and the abstractions to the build process.

As with the previous patch you'll need to apply the patch from #2815077-111: Adopt airbnb javascript style guide v13 as new baseline javascript coding standards for Drupal 8 core and contrib before applying this one.

drpal’s picture

Fixes the missing babel-core dependency.

Status: Needs review » Needs work

The last submitted patch, 7: 2868137-7.patch, failed testing.

drpal’s picture

Status: Needs work » Needs review
dawehner’s picture

Things seems to be a sane change ... the issue summary is not up to date though ...

+++ b/core/scripts/js/babel-es6-watch.js
--- /dev/null
+++ b/core/scripts/js/changeOrAdded.js

This file needs at least some documentation to explain what it is doing.

drpal’s picture

Issue summary: View changes
drpal’s picture

FileSize
27.66 KB
23.52 KB

Adding some comments about what's happening.

dawehner’s picture

+++ b/core/scripts/js/changeOrAdded.js
@@ -14,6 +16,7 @@
--- a/core/misc/drupal.js

--- a/core/misc/drupal.js
+++ b/core/misc/drupal.js

+++ b/core/misc/drupal.js
+++ b/core/misc/drupal.js
@@ -1,583 +1 @@

@@ -1,583 +1 @@
-})(Drupal, window.drupalSettings, window.drupalTranslations);
+window.Drupal={behaviors:{},locale:{}},function(c,d,f){c.throwError=function(g){setTimeout(function(){throw g},0)},c.attachBehaviors=function(g,h){g=g||document,h=h||d;var j=c.behaviors;for(var l in j)if(j.hasOwnProperty(l)&&'function'==typeof j[l].attach)try{j[l].attach(g,h)}catch(m){c.throwError(m)}},c.detachBehaviors=function(g,h,j){g=g||document,h=h||d,j=j||'unload';var l=c.behaviors;for(var m in l)if(l.hasOwnProperty(m)&&'function'==typeof l[m].detach)try{l[m].detach(g,h,j)}catch(n){c.throwError(n)}},c.checkPlain=function(g){return g=g.toString().replace(/&/g,'&amp;').replace(/"/g,'&quot;').replace(/</g,'&lt;').replace(/>/g,'&gt;'),g},c.formatString=function(g,h){var j={};for(var l in h)if(h.hasOwnProperty(l))switch(l.charAt(0)){case'@':j[l]=c.checkPlain(h[l]);break;case'!':j[l]=h[l];break;default:j[l]=c.theme('placeholder',h[l]);}return c.stringReplace(g,j,null)},c.stringReplace=function(g,h,j){if(0===g.length)return g;if(!Array.isArray(j)){for(var n in j=[],h)h.hasOwnProperty(n)&&j.push(n);j.sort(function(n,o){return n.length-o.length})}if(0===j.length)return g;var l=j.pop(),m=g.split(l);if(j.length)for(var n=0;n<m.length;n++)m[n]=c.stringReplace(m[n],h,j.slice(0));return m.join(h[l])},c.t=function(g,h,j){return j=j||{},j.context=j.context||'','undefined'!=typeof f&&f.strings&&f.strings[j.context]&&f.strings[j.context][g]&&(g=f.strings[j.context][g]),h&&(g=c.formatString(g,h)),g},c.url=function(g){return d.path.baseUrl+d.path.pathPrefix+g},c.url.toAbsolute=function(g){var h=document.createElement('a');try{g=decodeURIComponent(g)}catch(j){}return h.setAttribute('href',g),h.cloneNode(!1).href},c.url.isLocal=function(g){var h=c.url.toAbsolute(g),j=location.protocol;'http:'===j&&0===h.indexOf('https:')&&(j='https:');var l=j+'//'+location.host+d.path.baseUrl.slice(0,-1);try{h=decodeURIComponent(h)}catch(m){}try{l=decodeURIComponent(l)}catch(m){}return h===l||0===h.indexOf(l+'/')},c.formatPlural=function(g,h,j,l,m){l=l||{},l['@count']=g;var n=d.pluralDelimiter,o=c.t(h+n+j,l,m).split(n),p=0;return'undefined'!=typeof f&&f.pluralFormula?p=g in f.pluralFormula?f.pluralFormula[g]:f.pluralFormula['default']:1!==l['@count']&&(p=1),o[p]},c.encodePath=function(g){return window.encodeURIComponent(g).replace(/%2F/g,'/')},c.theme=function(g){var h=Array.prototype.slice.apply(arguments,[1]);if(g in c.theme)return c.theme[g].apply(this,h)},c.theme.placeholder=function(g){return'<em class="placeholder">'+c.checkPlain(g)+'</em>'}}(Drupal,window.drupalSettings,window.drupalTranslations);
\ No newline at end of file

Seems legit, no

dawehner’s picture

#2873888: package.json should specify an empty build step defines a way to specify a build step so the testbot can pick it up already.

drpal’s picture

FileSize
35.97 KB
32.5 KB

Remove the accidently included drupal.js file.

drpal’s picture

FileSize
58.03 KB
22.24 KB

See #17

drpal’s picture

FileSize
36.38 KB
21.57 KB

Again to remove the accidently included built files.

Adjusting this slightly to help with the proposed build process changes on commit and test bot changes.

Currently npm run build:js will process all files matching ./**/*.es6.js. This is not ideal if just one file was changed in the patch. With this change you can pass some extra arguments to build:js and only process the passed files.

// Only misc/drupal.es6.js misc/drupal.init.es6.js will be processed.
npm run build:js -- --files misc/drupal.es6.js misc/drupal.init.es6.js
alexpott’s picture

After the discussion about patch workflow and who builds the transpiled JS I tested out some of the suggestions - based on my findings documented here #2809281-17: Use ES6 for core JavaScript development but tldr - we have to commit the .js and the .js should be in the patches.

Therefore we shouldn't be minifying the JS. Fortunately that's simple - patch attached to do that.

Status: Needs review » Needs work

The last submitted patch, 18: 2868137-18.patch, failed testing.

drpal’s picture

+++ b/core/scripts/js/changeOrAdded.js
@@ -0,0 +1,26 @@
+      minified: false

In package.json scripts, we specify an environment variable, BABEL_ENV=production which triggers the Babel env preset. If we want to not actually minify code then we can just drop the BABEL_ENV variable from the run script.

droplet’s picture

  1. +++ b/core/package.json
    @@ -4,13 +4,14 @@
    +    "build:js": "BABEL_ENV=production node ./scripts/js/babel-es6-build.js",
    

    this https://www.npmjs.com/package/cross-env

  2. +++ b/core/package.json
    @@ -21,7 +22,25 @@
    +              "last 2 versions",
    

    specific version? A day shift makes huge diff :P

drpal’s picture

+++ b/core/package.json
@@ -21,7 +22,25 @@
+              "ie >= 11"

Since IE11 is the worst browser in this list, the last 2 versions doesn't really matter here. I wanted to leave it in for clarity though.

+++ b/core/package.json
@@ -4,13 +4,14 @@
+    "build:js": "BABEL_ENV=production node ./scripts/js/babel-es6-build.js",

I think we are going to drop this since we don't want to minify anyways.

I'll push up a patch soon to resolve the last point.

alexpott’s picture

@drpal

+++ b/core/scripts/js/changeOrAdded.js
@@ -0,0 +1,26 @@
+      sourceMaps: process.env.BABEL_ENV ? false : 'inline',

I think this line might need change too. Since if we're not minifying then source maps aren't really necessary. I'm not sure - not my area of expertise.

drpal’s picture

@alexpott

I wanted to include inline source maps when we're developing. It will make debugging much easier but are probably not needed in a production environment.

drpal’s picture

FileSize
29.07 KB
18.38 KB

Alright, dropping minification from babel-preset-babili and adjusting the creation of sourcemaps to be only inline during watching with cross-env NODE_ENV=development

drpal’s picture

FileSize
29.07 KB
289 bytes

Since #2842298: [policy, no patch] Drop IE9 and IE10 support from Drupal 8.4.x has not been resolved, dropping transpile settings back to IE9.

drpal’s picture

Status: Needs work » Needs review
alexpott’s picture

@drpal whilst I know that ie is always likely to be the least supported thing we support I think we should try to codify https://www.drupal.org/docs/8/system-requirements/browser-requirements rather than just have "last 2 versions", because as per https://kangax.github.io/compat-table/es6/ they all support different things.

drpal’s picture

@alexpott

Yeah, alright. I think this is a good idea. Since we unfortunately don't specify chrome or edge versions I just took a stab and specifying versions. I also fixed an issue where transpile errors were not actually being thrown.

Status: Needs review » Needs work

The last submitted patch, 29: interdiff-26-29.patch, failed testing.

drpal’s picture

Status: Needs work » Needs review
droplet’s picture

DEL.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I totally believe moving towards 'env' is the right step. Note: We can always iterate here and improve things over time.

  1. +++ b/core/scripts/js/babel-es6-build.js
    @@ -11,44 +11,32 @@
    +// Match only on .es6.js files.
    ...
    +// Ignore everything in node_modules
    

    Nitpick: Some of those comments are rather pointless, as they just explain what the code does. Often its more helpful to explain why this is done like that. Anyway that is more of a suggestion/personal style.

  2. +++ b/core/scripts/js/changeOrAdded.js
    --- /dev/null
    +++ b/core/scripts/js/log.js
    
    +++ b/core/scripts/js/log.js
    +++ b/core/scripts/js/log.js
    @@ -0,0 +1,4 @@
    
    @@ -0,0 +1,4 @@
    +module.exports = (message) => {
    +  // Logging human-readable timestamp.
    +  console.log(`[${new Date().toTimeString().slice(0, 8)}] ${message}`);
    +}
    

    Nice, our first real node module

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6154128 and pushed to 8.4.x. Thanks!

alexpott’s picture

  • alexpott committed 6154128 on 8.4.x
    Issue #2868137 by drpal, alexpott, dawehner, droplet: Improve JavaScript...

Status: Fixed » Closed (fixed)

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