-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Support a new web.browser.legacy platform to reduce bundle sizes for modern browsers. #9439
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
Conversation
@@ -0,0 +1,2 @@ | |||
require("core-js/modules/es7.string.pad-start"); | |||
require("core-js/modules/es7.string.pad-end"); |
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.
This set of core-js
polyfills is definitely too small for the current setMinimumBrowserVersions
constraints. Further consideration will be necessary, but I'm sure we can do (much) better than legacy.js
.
packages/meteor-base/package.js
Outdated
// decide whether to inject <script> tags into the <head> of the | ||
// response document to polyfill Web Sockets and/or ES5 support. | ||
'sockjs-shim', | ||
'es5-shim', |
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 should not have removed es5-shim
here, since the package still exists (unlike sockjs-shim
), and has no impact on modern browser bundles.
* @static | ||
* @type {Boolean} | ||
*/ | ||
isModern: config.isModern |
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 this actually useful? Maybe for collecting stats at runtime about how many clients qualified for the "modern" treatment?
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.
Could be useful. Would exposing it as a Meteor.platform
string (or maybe as a boolean, via a Meteor.isPlatform('<platform>')
) make it more useful in the event that additional platforms appear in the future? And to avoid too many Meteor.is${Platform}
variables?
|
||
- [ ] In development, save time by only rebuilding `web.browser` (modern)? | ||
|
||
- [ ] Try adding a `web.worker` platform and see if it works as expected. |
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.
👀😜 @mitar
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.
How hard it would be to allow web.worker.<tag>
?
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.
The main difficulty I see is that any package that adds something to client
currently would implicitly be adding that resource to web.worker
as well, since client
essentially means web.*
, which would include web.worker
and/or web.worker.*
.
That's a problem both because it would make the worker bundles larger and because existing package code might use global browser APIs that aren't available in web workers.
We could call it something different, like webworker.<tag>
, so it wouldn't inherit all the web
stuff, but you probably want to have at least some of your packages available in a worker.
Maybe packages have to opt into supporting workers with an isWorkerSafe: true
option in Package.describe
?
We also need worker-specific entry points in the application. Or maybe workers have to be represented as Meteor packages?
Now that I'm thinking about it, there are a lot of unanswered questions here.
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 would go with worker.<tag>
. Simple and backwards compatible. Those new packages which want to add things to worker, can do that.
BTW, then we should also add worker
directory besides client
and server
in apps? Or will this work just for packages?
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.
One solution is also that if I do api.use('package', 'worker.web')
and package
has only web
available, then it is still pulled in. So if package was meant to be used in the browser but has not been made available for workers, you could pull it in.
Also, is really a bundle still a thing with dynamic imports?
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.
Also, is really a bundle still a thing with dynamic imports?
If you use import()
, that just makes multiple bundles, one loaded up front, and some loaded later, right?
firefox: 45, | ||
mobile_safari: [9, 2], | ||
opera: 36, | ||
safari: 9, |
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.
Right now there's nothing to enforce the completeness of the browsers in this list, so there's a risk that someone could add a loose constraint for some other browser, thus accidentally causing it to receive the modern bundle, because it wasn't mentioned elsewhere with stronger constraints.
options.arch.split(".").slice(1).join("."); | ||
} else { | ||
this.urlPrefix = ""; | ||
} |
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.
/__browser/...
or /__browser.legacy/...
, which may be a breaking change for code that attempted to load these URLs without consulting the manifest. 🚨
The good news is that this code is now the single authoritative source of those URL prefixes, which is why very little other code (besides some tests) needed to change to accommodate the new URLs.
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.
Wow, this is awesome @benjamn! There is a lot going on here (as you know 🙂) so I'll have to review this in segments. I've added a few initial comments. Thanks!
@@ -0,0 +1,123 @@ | |||
const minimumVersions = Object.create(null); |
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'm wondering if we should consider exporting minimumVersions
? It might be nice to get programmatic access to the current minimumVersions
. For example, let's say we have someone accessing an app using an old browser, so isModern
is false
. We might then want to give them a heads up about the baseline browser versions they need to get past to get access to a better experience (so say dump the minimumVersions
on-screen in the app).
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 can totally imagine this example happening, but I think I'd like to wait until people ask for this feature. I suspect browser caching will hide the difference between modern and legacy bundles somewhat, so hopefully no one will complain? 🤞
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.
Sounds good - exposing less of the internals is always a good thing as well, so waiting to see what happens sounds like a plan. Thanks!
packages/modern-browsers/modern.js
Outdated
} | ||
|
||
// ECMAScript 2015 Classes | ||
setMinimumBrowserVersions({ |
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.
We're exporting setMinimumBrowserVersions
and allowing it to be called multiple times as needed which is great, but once all of the setMinimumBrowserVersions
calls have been completed and we have the new definitive minimumVersions
ready to go, what happens if at some point we notice the minimumVersions
aren't quite what we expected? How do we track down what call to setMinimumBrowserVersions
is impacting the minimum versions used, especially since setMinimumBrowserVersions
is part of the public API and can be called by 3rd party packages? The current values added via setMinimumBrowserVersions
don't have a feature identifier of any kind associated with them, so we don't have an easy programmatic way to track down what feature is causing the minimum versions to be set at a certain level. We have to either look at the source, track down inline comments (like ECMAScript 2015 Generator Functions
, ECMAScript 2015 Template Literals
), or rely on other forms of documentation. If the setMinimumBrowserVersions
are spread across multiple packages, this could be a pain. I'm wondering if we could somehow mark a certain minimum browser version set with an identifier, and track that identifier when setting the minimumVersions
baseline. So instead of
// ECMAScript 2015 Generator Functions
setMinimumBrowserVersions({
chrome: 39,
edge: 13,
firefox: 26,
mobile_safari: 10,
opera: 26,
safari: 10,
// Disallow any version of PhantomJS.
phantomjs: Infinity,
});
we have something like
setMinimumBrowserVersions('ECMAScript 2015 Generator Functions', {
chrome: 39,
edge: 13,
firefox: 26,
mobile_safari: 10,
opera: 26,
safari: 10,
// Disallow any version of PhantomJS.
phantomjs: Infinity,
});
then when deciding on the minimumVerions
, we track the identifier for the version that ended up taking priority and being set. So anyone with programmatic access to minimumVersions
could dump it out and see something like:
{
chrome: {
decidedBy: 'ECMAScript 2015 Classes',
version: 49,
},
edge: {
decidedBy: 'ECMAScript 2015 Generator Functions',
version: 13,
},
firefox: {
decidedBy: 'ECMAScript 2015 Classes',
version: 45,
},
...
}
Then again this might be over engineering ...
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 decided to do this by examining the call stack rather than passing in a string: 93c2632. What do you think?
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.
Even better idea @benjamn - this looks great!
9fc02a7
to
93c2632
Compare
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've run through the rest @benjamn - everything looks great!
fe4a37e
to
128f45f
Compare
@@ -79,7 +79,8 @@ BCp.processOneFileForTarget = function (inputFile, source) { | |||
extraFeatures.nodeMajorVersion = parseInt(process.versions.node); | |||
} | |||
|
|||
if (arch !== "web.browser.legacy") { | |||
if (arch !== "web.browser.legacy" && | |||
! hasOwn.call(extraFeatures, "runtime")) { | |||
extraFeatures.runtime = { |
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.
Specifically, this line from coffeescript-compiler
was being overridden here, before this change.
My latest thinking: the line between "modern" and "legacy" will essentially boil down to whether the client has native support for |
83c0a53
to
5db2bc6
Compare
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.
Since this is still labeled "WIP", I'll hold off on the using the GitHub "Approval" stamp (though I'm still approving of it!), but this looks great right now. I made a few comments within the review.
Generally speaking, getting Chrome Headless set up is obviously important here, but that should be easier with the restructuring in #9364. We don't currently have a lot of tests which utilize testWithAllClients
, but having CircleCI run both Chrome Headless and PhantomJS should be doable.
Since meteor self-test --browserstack
tests are still being problematic from CircleCI I ran them locally on this branch and got a varied results of results— for example, the versioning hot code push
test seems to pass on Firefox 57, Chrome 62, Safari 7.1, a Galaxy S7 "Browser", as well as IE9 but fails repeatedly on IE11. We should dig into nuances like that further.
(It's worth pointing out that when I randomly picked the versioning hot code push
test, it was misconfigured for testWithAllClients
. PR incoming to address that.)
packages/modern-browsers/README.md
Outdated
@@ -0,0 +1,7 @@ | |||
# modern |
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.
Should this be # modern-browsers
?
* @static | ||
* @type {Boolean} | ||
*/ | ||
isModern: config.isModern |
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.
Could be useful. Would exposing it as a Meteor.platform
string (or maybe as a boolean, via a Meteor.isPlatform('<platform>')
) make it more useful in the event that additional platforms appear in the future? And to avoid too many Meteor.is${Platform}
variables?
// retrying the connection. | ||
lastError && ! hasSockJS && | ||
import("./sockjs-0.3.4.js") | ||
).done(() => { |
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 understand the purpose it serves here, but I didn't realize our Promise
implementation had Promise.prototype.done
!
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.
Yep, I made sure it was there when I refactored the promise
package to use the legacy system.
packages/babel-compiler/versions.js
Outdated
// configuration. | ||
Package["modern-browsers"].setMinimumBrowserVersions( | ||
Babel.getMinimumModernBrowserVersions(), | ||
"packages/babel-compiler/versions.js" |
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, setMinimumBrowserVersions
's source
parameter is free-form, and modules
isn't available in babel-compiler
(thus no module.id
to use here), but would it make any sense to have this hand-coded string be more consistent with what the module.id
would have been by other setMinimumBrowserVersions
registrants? (Which I think would be /node_modules/meteor/babel-compiler/versions.js
?)
Since this test utilizes the `testWithAllClients` technique, which runs the tests in various clients/browsers, it's necessary for the tests `Sandbox` to define `clients`, otherwise the function within `testWithAllClients` will not be executed at all. This was causing this particular test to always return success (it was running without failure on exactly zero clients). Also the technique of setting `this.baseTimeout` appeared to cause problems, likely because it overrides various other values instead of using `waitSecs` (we don't use the `baseTimeout` technique in other places within self-tests either). Discovered during testing, as mentioned in #9439 (review).
9622dcc
to
87d0562
Compare
Just as the URLs of static JS resources depend on request.browser, so too must dynamic modules be loaded from the correct build directory based on the user agent string of the __meteor__/dynamic-import/fetch HTTP request.
In my research, I found the data used by @babel/preset-env to be more conservative than necessary, so I have not followed their minimum version constraints exactly. For example, every feature of the ECMAScript `Map` API is clearly supported in Firefox 45+, according to MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Browser_compatibility However, @babel/preset-env requires `core-js/modules/es6.map` in any version of Firefox earlier than 53: https://github.com/babel/babel/blob/e270fbe7f0f47af22af462c18196c11d7a870c67/packages/babel-preset-env/data/built-ins.json#L117 Since I can't find any evidence that @babel/preset-env knows better than other sources (I think they might just be using a compatibility table that doesn't go back far enough), I have tentatively trusted MDN in picking these versions. If any bugs are ever reported due to this choice of versions and polyfills, we have two options to fix them: * Tighten the minimum version constraints so that the affected browsers are considered legacy instead of modern. * Include the missing `core-js` polyfills for all modern browsers.
This led to a regression in coffeescript-test-helper because the truthy extraFeatures.runtime property allowed require("@babel/runtime/...") to appear in a compiled CoffeeScript file, though require was not defined. If @GeoffreyBooth agrees, we could have the coffeescript package api.imply("modules"), which would make require would work, but I seem to remember we had some reasons for not doing that previously.
Calling getCaller was noticeably slowing down server startup, unfortunately. cc @hwillson
https://github.com/meteor/babel-preset-meteor/blob/master/modern.js https://github.com/meteor/babel/blob/dfcce32868/options.js#L81 With the minimum versions from babel-preset-meteor/modern-versions.js, the difference between "modern" and "legacy" browsers boils down to supporting native async functions. I'm eager to stop compiling generator functions with Regenerator, and none of the transforms that compile async functions to native generator functions seem much better than the Regenerator experience (source maps are still wonky within the async function, so it's obvious you're not working with native code). Native async functions are supported by 73% of desktop and mobile browsers worldwide (81% in the US), and that percentage is only going to keep increasing. I see no reason not to embrace this future now.
I've decided to stick with Meteor.isModern for now, since it's false for both web.browser.legacy and web.cordova bundles, which would make logic involving a hypothetical Meteor.platform property more complicated than simply using Meteor.isModern. I'm open to revisiting this later.
7397e8f
to
f878ba1
Compare
@GeoffreyBooth Sorry for re-notifying you with that question, though I very much appreciate your response. I recently rebased and force-pushed this branch, which I think is why you got @-mentioned again. I'm open to making |
I missed these packages when publishing 1.6.2-beta.0, since I hadn't bumped their versions in #9439 as I should have, so the release script did not republish them. Because of this, 1.6.2-beta.0 will not be usable, and so I will publish 1.6.2-beta.1 ASAP. The boilerplate-generator version bump is patch rather than minor, since the relevant changes to that package seemed backwards-compatible.
…kage. Because Promise.asyncApply is only defined on the server, and Meteor 1.6 no longer uses Regenerator to compile async/await and generator functions on the server, this code no longer serves any meaningful purpose. On top of that, the babel-runtime.js module is loaded on the client, so this code was forcing the Regenerator runtime to be included in the client JS bundle, even if generator functions were not used anywhere else in the application. The benefit of removing this @babel/runtime/regenerator dependency won't be fully apparent until Meteor 1.6.2, since there are probably other places in client code that depend on it, so it will probably still be bundled in most applications. However, the new web.browser.legacy system (#9439) should remove most of those dependencies for modern browsers, as Meteor 1.6.2 will no longer use Regenerator to compile async/await and generators for the modern JS bundle.
@benjamn I think this is already happening, but as part of testing this it would be nice if packages that use |
During the Meteor 1.6.1 beta period, we introduced logic to render a <script> tag to load the SockJS library in older browsers (#9353), and so it seemed important to run test-packages both with and without the <script> tag, using a special query parameter appended to the app URL. The #9353 changes were ultimately reverted before Meteor 1.6.1 was released (see 3658042), and Meteor 1.6.2 will take a very different approach to bundling dependencies like SockJS for legacy browsers (#9439). As part of this approach, PhantomJS is always considered a legacy browser, and as such provides valuable feedback on the behavior of web.browser.legacy bundles. However, since there's nothing to configure with regard to SockJS anymore, there's no point in running the test-packages suite twice in PhantomJS. In order to run these tests in a modern browser environment, we should probably revisit the idea of running tests in headless Chrome: meteor/meteor-feature-requests#254
This PR allows Meteor to send different static resources to "modern" and "legacy" browsers, where the "modern" threshold is collaboratively defined by any code wishing to participate in the new system.
The motivation for these changes is to allow the vast majority of application users (80%+) to enjoy the full benefits of modern evergreen browsers, without being weighed down by the needs of older 'nevergreen' browsers. At the same time, those older browsers will still receive all the polyfills, syntax transforms, and other workarounds that they need, completely automatically.
Specifically, an additional target architecture has joined the existing
os
,web.browser
, andweb.cordova
architectures: namely,web.browser.legacy
, which includes any resources added toweb.browser
, but may also include additional resources of its own to support older browsers.This differential bundling technique supersedes our previous strategy of server-rendering certain polyfill scripts earlier in the HTML document (#9353, #9360), though it has essentially the same goal: to eliminate useless code from modern JS bundles, without breaking older browsers in the process.
As a straightforward example, here's a commit that removes all
Promise
polyfill code from the modern JavaScript bundle, while continuing to include it in the legacy bundle: 4609a57Note especially the call to
setMinimumBrowserVersions
, which allows thepromise
package to enforce its own assumptions about the minimum requirements for "modern" browsers:These minimum versions are relatively ancient, since
Promise
s have been natively supported in most browsers for longer than many other ECMAScript features. In other words, the actual threshold betweenweb.browser
andweb.browser.legacy
is likely to be determined by other calls tosetMinimumBrowserVersions
, but that's OK, because thepromise
package will work with or without polyfills in any browser that meets the requirements above.More exciting examples include using vastly fewer
core-js
polyfills in theecmascript-runtime-client
package, and running entirely different Babel plugins in thebabel-compiler
package, so that browsers with native support for generator functions (for example) no longer need them to be compiled them with Regenerator, and no longer need to load theregenerator-runtime
package.While it was tempting to target even finer-grained categories of browser versions (more than just
web.browser
andweb.browser.legacy
), the browser landscape today thankfully does not demand that kind of specialization. More internet users than ever have access to browsers that need very few polyfills and Babel plugins, and that percentage will only keep growing. Users who still can't use a modern browser will experience slower initial page load times, but those differences will mostly disappear thanks to browser caching, are are likely to be overshadowed by a host of other performance problems in older JS engines.Perhaps even more importantly, different configurations of polyfills and plugins must be tested rigorously, and testing two configurations is much easier than testing a whole matrix of slightly different configurations. Not to mention, it would take more time to build lots of different bundles, so building just two bundles is a bit more manageable.
This is still a work in progress, primarily because we still need to
core-js
polyfills for theecmascript-runtime-client
package, and enforce those assumptions withsetMinimumBrowserVersions
.web.browser
vs.web.browser.legacy
.