Problem/Motivation

Core JS can be improved; a lot of the JS ecosystem is moving to the new version of JS (Angular, React, etc.). There are many, many DX improvements. Our JS is not prepared to be unit-tested, and moving to ES6 and modules is a good occasion to refactor some code so that it is unit-testable. For informations about which browser support what see Kangax compatibility table, essentially everything we'll change except modules is supported on latest version of all browsers, except IE11.

Proposed resolution

In order to modernize our JavaScript we are considering using ES6 for core development. It would mean that writing a JavaScript patch for core would require the use of Node.js and Babel to transpile ES6 to ES5. All the configuration would be provided by core (we would only need npm install to get started). A nice side effect of using a preprocess/compile step is that it makes it easy to add polyfills for unsupported browsers (allowing projects to support older browsers than core does).

API changes

There is no change for end users and module developers. This will only impact core JS developers. Contrib will be able to use ES6 if they like but it won't be required.

We might be able to do this without an API change (depending on how 'API' is defined).

Core workflow changes

Using ES6 means that we require Node.js and Babel to be able to contribute to core JavaScript.

Talked with alexpott and for now we'll say a JavaScript patch should include the changes to the ES6 file as well as the changes to the ES5 generated file(s).

Plan

  1. Add a minimal package.json file to drupal (it's a composer.json for js) #2809165: Add package.json to Drupal
  2. Write scripts to rename all our *.js to files to *.es6.js and commands to compile those files to regular JS we will use. No libraries.yml change #2809477: Add ES6 to ES5 build process and #2818825: Rename all JS files to *.es6.js and compile them
  3. Formally adopt ES6 Airbnb coding standards #2815077: Adopt airbnb javascript style guide v13 as new baseline javascript coding standards for Drupal 8 core and contrib
  4. Run codemods with jscodeshift on our *.es6.js files to actually make use of ES6 features: template litterals, const and let, etc. (it's a coder upgrade for js) #2809735: Use jscodeshift for ES3/5 to ES6 transform in CORE & Contributed modules
  5. Choose unit test suite (Airbnb uses Mocha) and refactor one script (vertical tabs for exemple?) and write unit tests for it #2815199: Add tools and scripts for writing and running javascript unit tests
  6. Separate all the 'library' code from behaviors code [#]
  7. Refactor all the libraries so that they are unit-testable [#]

Things to do while we're at it to help keep things clean:

DX improvements

Event if we don't make use of module definitions, there are many things we can use to improve our JS, for example:

for (let element of document.querySelectorAll('div')) {
  // instead of $('div').each()
}

Object.assign({}, settings); 
// instead of jQuery.extend({}, settings);

function destructuring({url, type, options}) {
  // instead of var url = param.url; var param.type; …
}

function defaultValues(context = document, trigger = 'unload') {
  // instead of context = context || document; trigger = trigger || 'unload';
}

var templateString = `<pre>${element.innerHTML}</pre>`;

Promise // <- all good

For more information:

With all the talk about frameworks and getting people to contribute to core, using ES6 is a real opportunity to get more people. At least more than any framework. Again, this will impact people working on core patches only. A git checkout of Drupal will contain the ES5 version of the JavaScript to be run as-is, and Node.js wouldn't be required.

Comments

nod_ created an issue. See original summary.

Wim Leers’s picture

Issue summary: View changes
Issue tags: +JavaScript
droplet’s picture

I thought to introduce NODEJS development env in CSS (#2658650: [META] Optimize Frontend Workflow in Core Development) first, it's more straightforward. Now we do it in JS! well-done GUYS!! I'll follow your footprint!

mpdonadio’s picture

Does part of the plan need to include picking a ES6 polyfill library (eg, https://github.com/paulmillr/es6-shim/) or would this be part of the tooling choice?

prestonso’s picture

Title: Use ES6 for core javascript developement » Use ES6 for core JavaScript development
Issue summary: View changes

A few fixes to IS and title.

nod_’s picture

Issue summary: View changes
droplet’s picture

nod_’s picture

Issue summary: View changes
nod_’s picture

Issue summary: View changes
Related issues: +#2809477: Add ES6 to ES5 build process

Updated the plan to reflect what's going on

Plan

  1. Add a minimal package.json file to drupal (it's a composer.json for js) #2809165: Add package.json to Drupal
  2. Write scripts to rename all our *.js to files to *.es6.js and commands to compile those files to regular JS we will use. No libraries.yml change #2809477: Add ES6 to ES5 build process
  3. Formally adopt ES6 Airbnb coding standards (I think we are already compatible) [#]
  4. Run codemods with jscodeshift on our *.es6.js files to actually make use of ES6 features: template litterals, const and let, etc. (it's a coder upgrade for js) #2809735: Use jscodeshift for ES3/5 to ES6 transform in CORE & Contributed modules
  5. Choose unit test suite (Airbnb uses Mocha) and refactor one script (vertical tabs for exemple) and write unit tests for it [#]
  6. Separate all the 'library' code from behaviors code [#]
  7. Refactor all the libraries so that they are unit-testable [#]

Things to do while we're at it to help keep things clean:

nod_’s picture

nod_’s picture

Issue summary: View changes
nod_’s picture

nod_’s picture

Issue summary: View changes
nod_’s picture

Issue summary: View changes

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.

finne’s picture

Adopt airbnb JS code style is now RTBC... Let's try and move forward. Would be so nice to have in core. In the (near) future: IE 11 will be the only browser supported by Drupal 8.4 (see Drop IE9 and IE10 support from Drupal 8.4.x) that needs ES5. All others can do native ES6 without transpiling.
(I am a recently converted ES6 enthusiast: https://www.dx-experts.nl/its-time-es6 )

alexpott’s picture

At Drupalcon Baltimore we discussed getting the following three patches done:

One of the issues that came up was what should the process look like once these patches had landed. We agreed that we have to commit the transpiled JS to core because otherwise we would be breaking every ones build process because in order to have a working Drupal you need to run yarn install and yarn run build:js.

If we add the transpiled JS to git this is very problematic for a patch (or PR) workflow because non-conflicting changes to the source es6 javascript will result in conflicting changes to the transpiled JS. @dawehner suggested that maybe a composer library could supply the transpiled JS but then there is the problem of how that gets updated and built. Also using composer to deliver JS is kinda weird. I suggested that maybe we could add steps to composer install to do the yarn install and build - but this is also problematic because there would be new dependencies to composer install that can not be got using composer. @xjm suggested that core committers could be responsible for building JS and committing the transpiled JS in a separate commit along side the commit of the patch.

I've started implementing @xjm's suggested workflow in https://github.com/alexpott/d8githooks/tree/js-builder - this means core committers need yarn but that is a manageable problem.

The one concern I have after working on the problem and testing the flow is this.

  1. Contributor starts to work on javascript issue.
  2. They are using the build system locally to build the js. It's all working great. Time to create a patch. They commit their changes locally and do git diff 8.4.x HEAD > some_nice.patch. Create a patch without the transpiled assets.
  3. Issue gets to rtbc
  4. Committer applies patch - and commits it. The build process and triggered and the post-commit hook commits the transpiled files...

The bit that is problematic is the Create a patch without the transpiled assets.
As we're putting the files in git I can't think of an obvious way to ignore the changes. .gitignore will ignore files but if that file then is forced into a repository it'll stop being ignored regardless. All I've come up with so far is adding a script to package.json that will help contributors create "correct" js patches - i.e. ones that ignore the changes to transpiled files.

alexpott’s picture

One other idea is based on something that I think @dawehner said in the meeting. Instead of the build step producing minified and uglified js in Drupal 8, we drop that idea and produce readable JS that is also patchable and just suffers conflicts if the es6 change would also conflict.

alexpott’s picture

I think we should do #18 as it is totally possible - see #2868137-18: Improve JavaScript build tooling.. This means the patch flow does not change - and we should file a Drupal 9 task to remove .js from the codebase and introduce a problem build process for it that everyone will have to do.

droplet’s picture

To take it seriously, our reviewers should testing on the final compiled JS all time.

Problem 1 is a minor version of Babel updates would affect the compiled JS. It's one of the reason Facebook trying to build Yarn.

Problem 2 is any version of Babel (build toolings) updates may invalid all JS patches on d.org

Problem 3 is I think 99.9% of the time contributors won't do git pull & Yarn before their contributions. At least, I'm the one.

At some point, I think the d.org server should compile the files for reviewers & core committers. It's the only smooth way to guarantee we testing on the up-to-date compiled JS. (less human reroll game)

Before d.org automated build, Core committers still can't skip their build process because of Point 1, 2, 3 above

dawehner’s picture

As we're putting the files in git I can't think of an obvious way to ignore the changes. .gitignore will ignore files but if that file then is forced into a repository it'll stop being ignored regardless. All I've come up with so far is adding a script to package.json that will help contributors create "correct" js patches - i.e. ones that ignore the changes to transpiled files.

Oh, interesting point. On projects I work around this particular problem using git update-index --assume-unchanged , but in this case, this trick would require anyone who is working on core to do the same steps. To be honest I don't have any clever suggestion how just the core committers could deal with it.

Problem 3 is I think 99.9% of the time contributors won't do git pull & Yarn before their contributions. At least, I'm the one.

I think this is a culture problem which can be solved by education over time.

droplet’s picture

Create a patch without the transpiled assets.

Ignore files / Conflicts:

git diff -- . ":!*.js" // Not a require, we still want our reviwers testing it without build process, right?
git apply bad.patch --exclude=*.js // 100% Core committers & d.org  testbots

To take it seriously again, (before d.org automated build)

I'd suggest every reviewer & core committers this workflow

1. git apply --index bad.patch
2. build process (run watcher.js once only per contribution session)
3. git diff (// with diff, Yo, you caught! Please use latest build Tool! :p) (this is also the interdiff if you ever made changes to the patch you working on)

mpdonadio’s picture

To take it seriously, our reviewers should testing on the final compiled JS all time.

Manual testing yes, but code review no. So, we need to be able to support both.

Problem 3 is I think 99.9% of the time contributors won't do git pull & Yarn before their contributions. At least, I'm the one.

I think we can solve this with docs and other eduction as @dawehner mentioned. Most people doing active core contributions should have `composer install` as a post-merge hook anyway. Adding yarn / gulp / make / whatever to this isn't a huge deal.

cilefen’s picture

At some point, I think the d.org server should compile the files for reviewers & core committers. It's the only smooth way to guarantee we testing on the up-to-date compiled JS. (less human reroll game)

Yes.

catch’s picture

For #18 we have multiple issues for minification/uglification of js files on the fly and can defer that to those #1014086: Race conditions, stampedes and cold cache performance issues with css/js aggregation / #1048316: [meta] CSS and Javascript aggregation improvement are the issues. Those issues or linked ones also have good arguments for not doing it on a build step per-file fwiw.

Also I have to ask:

If transpiling is only necessary for IE11, if we transpile in packaging, and encourage people to transpile for local/composed builds, but don't commit transpiled assets to core, then can we not serve non-transpiled assets that will work on most modern browsers as the fallback? We could have a hook_requirements() to detect the non-transpiled situation. I feel like people who are already doing composed builds can handle adding a step tbh. Or are there features we want to use that aren't yet implemented outside of IE11 too?

droplet’s picture

Or are there features we want to use that aren't yet implemented outside of IE11 too?

I think IE11 isn't the only one. some ES6 still not supported in FF 45 ESR..etc.

then can we not serve non-transpiled assets that will work on most modern browsers as the fallback

ES2015, ES6, ES Next. It's almost the same thing to nowaday's JS Developers. It creates an unreviewable state and buggy on diff Modern browsers.

A common workflow is compiling it at each release (git tag)

finne’s picture

I think IE11 isn't the only one. some ES6 still not supported in FF 45 ESR..etc.

Looking at the ES6 support https://kangax.github.io/compat-table/es6/ and the FF ESR usage share https://en.wikipedia.org/wiki/Template:Firefox_usage_share I would say that although there is certainly an issue with FF ESR, the small usage percentage does not warrant a lot of effort on Drupal's part.

I would guess some FF ESR users (IT departments at organisations mostly) also deploy a fixed version of Chrome, but this is not as easily seen in statistics.

Apart from IE and FF ESR all other platforms/browsers have such robust update systems that most users of these browsers are automatically pushed to the latest release. Looking at the current native support for ES6 I would summarise for D8.4 browsers:

  • Safari mobile/desktop: 100%
  • Chrome/Opera/Firefox mobile/desktop: 97% (missing only support for tail calls)
  • Edge 96% (missing only support for tail calls and some other calls are currently only supported with a flag)
  • IE 11% (not getting support for ES6)

NB: no browser supports ES6 Import/module statements natively.

Microsoft is working on Edge https://developer.microsoft.com/en-us/microsoft-edge/platform/status, and soon all ES6 features will be supported except Tail Calls.

So looking in more detail at this I would conclude that pure ES6 (with the exception of Tail Calls and imports) will be natively supported by all D8.4 browsers except IE11. Maybe Edge will not be there yet when D8.4 is released, but it could also be the other way around :-).

Not saying Drupal should not offer a transpiled version of JS assets, but offering just the ES6 native code as a fallback looks like a good option to me.

alexpott’s picture

@catch I don't understand how we can transpile the js to the same location. Which as far as I can see is what your suggestion in #25 would mean. The problem is our asset libraries only point to one location. I guess we could add yet another location for them search. Thinking about drupal.system which is defined as so:

drupal.system:
  version: VERSION
  js:
    js/system.js: {}
  dependencies:
    - core/jquery
    - core/drupal
    - core/drupalSettings
    - core/jquery.once

If you have any theme overrides it use them first and then it would search some new transpiled location - maybe core/modules/system/js/transpiled - and it looks in there for core/modules/system/js/transpiled/system.js and finally it falls back to core/system/js/system.js which is the es6 version.

Still I don't really see how this is better than shipping the transpiled js as core/system/js/system.js and the es6 as core/system/js/system.es6.js and committing the built versions. As #27 shows other browsers that we plan to support probably will not have full support in last two supported releases when 8.4.0 is released.

Committing the transpiled js has several advantages as far as I can see:

  • Every Drupal 8 build process is still valid
  • For developers - just they have to learn how to use yarn to build the transpiled JS but that's a learning curve that is solveable
  • For developers - once they've learnt to transpile the patch flow is just then same.
  • We don't have to change DrupalCI and d.o packaging and we can come up with a proper plan for this in Drupal 9.
  • For reviewers / running the patch of Simplytest.me they have the final patch - they can apply and test with no build step.
  • For reviewers / committers they don't have to build although a JS review step should be to check that the JS has been transpiled correctly
Mile23’s picture

Some conversation about how to standardize front end libraries for contrib here: #2873160: Drupal core should help make 3rd party library management not torturous

alexpott’s picture

Discussed the ES6 build process with @catch. We agreed that we would commit the transpired assets for now (this is not a commitment to keep them in Drupal 8 forever). Once we've committed https://www.drupal.org/node/2815077, https://www.drupal.org/node/2868137 and https://www.drupal.org/node/2818825 then javascript patches will have to include the transpired assets.

The rationale for this decision is:

  • The libraries system points to a single asset - not both the transpiled and ES 6 versions.
  • We want progress on ES6 without packaging workflow - this can come later.
  • Minimal changes to the patch workflow. Regardless of what we do, in order to test locally you will need to transpile so requiring this is just part of the process of moving to ES6.
  • This means that people's custom build processes can experiment will transpiling before we require it.
catch’s picture

Regardless of what we do, in order to test locally you will need to transpile so requiring this is just part of the process of moving to ES6.

My only quibble would be that if we eventually implement a non-transpiled fallback for library processing, then it'll be possible to test locally on modern browsers without transpiling, but with the status quo this is true.

Otherwise +1 to this plan, as long as we can iterate on it later - I don't think we have to provide bc between minor releases for build process, we already removed /vendor (and that was even in a patch release) which required changes to people's workflows.