-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Add createModernBuild option #188
Conversation
Closes #183.
It looks to me like if we wanted to remove compilation for template literals, default parameters, and for-of loops, all we'd need to do is bump the Edge version to 14. |
Object.assign({}, batfishConfig, { | ||
babelPresetEnvOptions: { | ||
useBuiltIns: true, | ||
targets: { node: 'current' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we only need to compile what's necessary for the current version of Node. Can you explain why this is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we're using Node 6, we get a lot of ES2015 without compilation. So we should in theory have faster compile-time and maybe even faster runtime if, for example, we don't compile classes and arrow functions, which Node 6 understands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the set of ES features that get compiled, and end up in the static bundle, are driven by the version of Node that compiles the assets? If I'm understanding this correctly, I feel like this could result in some confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we clarified in chat: The misunderstanding here was about where and when the static bundle runs. The output of this Webpack task is just static-render-pages.js
, and that only runs in Node.
try { | ||
eval('const a = x => 2; class Foo {}'); | ||
supportsModern = true; | ||
} catch (e) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for the e
argument (is it even an argument?)? Just curious!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only to please ESLint 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh I see!
This is super cool, and is working as expected. I say it's ready for the next step. Nice work! |
@@ -74,7 +70,7 @@ function createWebpackConfigBase( | |||
presets: babelPresets, | |||
plugins: babelPlugins, | |||
babelrc: false, | |||
compact: true | |||
compact: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this back to avoid those annoying Webpack messages about deoptimization.
Object.assign({}, batfishConfig, { | ||
babelPresetEnvOptions: { | ||
useBuiltIns: true, | ||
targets: { node: 'current' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we clarified in chat: The misunderstanding here was about where and when the static bundle runs. The output of this Webpack task is just static-render-pages.js
, and that only runs in Node.
Once Babel 7 is out I'm going to start clean on this idea. |
@davidtheclark In case you find it helpful, here's a blog post about how Meteor 1.7 implements modern/legacy bundling: https://blog.meteor.com/meteor-1-7-and-the-evergreen-dream-a8c1270b0901 |
Closes #183. (Also, for kicks, closes #187.)
Inspired by https://philipwalton.com/articles/deploying-es2015-code-in-production-today/.
Adds an option
createModernBuild
, which defaults totrue
. When this mode is on, Batfish turns up the fanciness ...I'm considering a "modern" build one that expects the browser to support ES2015 classes and arrow functions. I was thinking that no compilation of those two features might save us a lot, considering how often they are used and how bulky the class compilation is. However, there are some other ES2015 features we might want to expect, as well, for similar reasons:
Batfish hard-codes a
babel-preset-env
configuration that would create a build suited to browsers with the expected ES2015 features (see https://github.com/mapbox/batfish/compare/es2015-bundle?expand=1#diff-1fe5c18317c115506c68d98801aba837R17).When you
batfish start
, it uses that config, assuming you're going to be doing your iterative development with a modern browser.When you
batfish build
, Batfish builds two sets of client bundles, one for modern browsers and one for legacy browsers. (It only needs to build one static bundle still, but while investigating this I realized we can limit compilation on that static bundle to the features not available in your running version of Node.) So we end up with two copies of almost every file, the modern versions prefixed withm-
. When we build the HTML, then, we include a runtime check for support of the ES2015 syntax we want (i.e. classes and arrow functions): if it's supported, load the modern bundles; if not, load the legacy bundles.Another trick is that Uglify does not yet understand ES2015 syntax, so when minifying the modern bundles we need to use babel-minify, instead.
I've tried this out on several examples, using IE11 as the legacy browser — and it seemed to work nicely!
@jfurrow I'd love your review — affirmation that this makes, or criticism of it — before I update all the relevant tests.