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 v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib is finalized.
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff-26-29.patch | 908 bytes | GrandmaGlassesRopeMan |
#29 | 2868137-29.patch | 29.24 KB | GrandmaGlassesRopeMan |
#26 | interdiff-25-26.txt | 289 bytes | GrandmaGlassesRopeMan |
#26 | 2868137-26.patch | 29.07 KB | GrandmaGlassesRopeMan |
#25 | interdiff-18-25.txt | 18.38 KB | GrandmaGlassesRopeMan |
Comments
Comment #3
GrandmaGlassesRopeManComment #4
droplet CreditAttribution: droplet commentedThanks @drpal, I waited for other comments for few day. Unfortunately no comments. Here's my little thoughts.
@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.
Comment #5
GrandmaGlassesRopeMan@droplet
Sure.
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 forObject.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 ofcore-js
with arequire/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.
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.
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.Comment #6
GrandmaGlassesRopeManI'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 v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib before applying this one.
Comment #7
GrandmaGlassesRopeManFixes the missing
babel-core
dependency.Comment #9
GrandmaGlassesRopeManComment #10
dawehnerThings seems to be a sane change ... the issue summary is not up to date though ...
This file needs at least some documentation to explain what it is doing.
Comment #11
GrandmaGlassesRopeManComment #12
GrandmaGlassesRopeManAdding some comments about what's happening.
Comment #13
dawehnerSeems legit, no
Comment #14
dawehner#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.
Comment #15
GrandmaGlassesRopeManRemove the accidently included
drupal.js
file.Comment #16
GrandmaGlassesRopeManSee #17
Comment #17
GrandmaGlassesRopeManAgain 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 tobuild:js
and only process the passed files.Comment #18
alexpottAfter 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.
Comment #20
GrandmaGlassesRopeManIn
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 theBABEL_ENV
variable from the run script.Comment #21
droplet CreditAttribution: droplet commentedthis https://www.npmjs.com/package/cross-env
specific version? A day shift makes huge diff :P
Comment #22
GrandmaGlassesRopeManSince 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.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.
Comment #23
alexpott@drpal
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.
Comment #24
GrandmaGlassesRopeMan@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.
Comment #25
GrandmaGlassesRopeManAlright, dropping minification from
babel-preset-babili
and adjusting the creation of sourcemaps to be only inline during watching withcross-env NODE_ENV=development
Comment #26
GrandmaGlassesRopeManSince #2842298: [policy, no patch] Drop IE9 and IE10 support from Drupal 8.4.x has not been resolved, dropping transpile settings back to
IE9
.Comment #27
GrandmaGlassesRopeManComment #28
alexpott@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.Comment #29
GrandmaGlassesRopeMan@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.
Comment #31
GrandmaGlassesRopeManComment #32
droplet CreditAttribution: droplet commentedDEL.
Comment #33
dawehnerI totally believe moving towards 'env' is the right step. Note: We can always iterate here and improve things over time.
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.
Nice, our first real node module
Comment #34
alexpottCommitted 6154128 and pushed to 8.4.x. Thanks!
Comment #35
alexpott