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

Support fetching dynamic modules via HTTP POST requests. #9384

Merged
merged 14 commits into from Nov 17, 2017

Conversation

Projects
None yet
4 participants
@benjamn
Member

benjamn commented Nov 17, 2017

Ever since Meteor 1.5 first shipped, dynamic modules have been fetched over a WebSocket, which is appealing because sockets have very little latency and metadata overhead per round-trip.

However, using a WebSocket requires first establishing a socket connection to the server, which takes time and may require a WebSocket polyfill.

An even more subtle problem is that we cannot use dynamic imports in any of the code that implements the ddp-client package, as long as the dynamic-import package depends on ddp-client.

By switching from WebSockets to HTTP POST requests, this commit radically reduces the dependencies of the dynamic-import package, while preserving (or even exceeding) the benefits of socket-based dynamic module fetching:

  • The client makes a single HTTP POST request for the exact set of dynamic modules that it needs, which is much cheaper/faster than making several/many HTTP requests in parallel, with or without HTTP/2.

  • Because the client uses a permanent cache (IndexedDB) to avoid requesting any modules it has ever previously received, the lack of HTTP caching for POST requests is not a problem.

  • Because the HTTP response contains all the requested dynamic modules in a single JSON payload, gzip compression works across modules, which produces a smaller total response size than if each individual module was compressed by itself.

  • Although HTTP requests have more latency than WebSocket messages, the ability to make HTTP requests begins much sooner than a WebSocket connection can be established, which more than makes up for the latency disadvantage.

  • HTTP requests are a little easier to inspect and debug in the dev tools than WebSocket frames.

  • The new /__dynamicImport HTTP endpoint is a raw Connect/Express-style endpoint, so it bypasses the Meteor method-calling system altogether, which eliminates some additional overhead.

  • Fetching dynamic modules no longer competes with other WebSocket traffic such as DDP and Livedata.

  • Although the implementation of the /__dynamicImport endpoint is a bit too complicated to allow serving dynamic modules from a CDN, fetching modules individually from a CDN remains a possibility for future experimentation. In other words, how modules are fetched is still just an implementation detail of the meteorInstall.fetch callback.

  • As with the WebSocket implementation, the module server is totally stateless, so it should be easy to scale up if necessary.

I wish I had appreciated the advantages of an HTTP-based implementation sooner, but I think the transition will be pretty seamless, since the new implementation is completely backwards compatible with the old.

benjamn added some commits Nov 16, 2017

Allow Meteor.startup to work in isopackets and build plugins.
Since global.__meteor_bootstrap__ is not defined in plugins, the callback
will simply be called immediately.
Remove underscore from http package, and use modules.
I replaced the ecmascript package with just the modules package so that
ecmascript and its dependencies can depend on http without creating
package dependency cycles.

I also took this opportunity to upgrade the `request` npm dependency to
its latest version (2.83.0), and removed the `deprecated.js` file, which
used to define `Meteor.http`.
Support fetching dynamic modules via HTTP POST requests.
Ever since Meteor 1.5 first shipped, dynamic modules have been fetched
over a WebSocket, which is appealing because sockets have very little
latency and metadata overhead per round-trip.

However, using a WebSocket requires first establishing a socket connection
to the server, which takes time and may require a WebSocket polyfill.

An even more subtle problem is that we cannot use dynamic imports in any
of the code that implements the ddp-client package, as long as the
dynamic-import package depends on ddp-client.

By switching from WebSockets to HTTP POST requests, this commit radically
reduces the dependencies of the dynamic-import package, while preserving
(or even exceeding) the benefits of socket-based dynamic module fetching:

1. The client makes a single HTTP POST request for the exact set of
   dynamic modules that it needs, which is much cheaper/faster than making
   several/many HTTP requests in parallel, with or without HTTP/2.

2. Because the client uses a permanent cache (IndexedDB) to avoid
   requesting any modules it has ever previously received, the lack of
   HTTP caching for POST requests is not a problem.

3. Because the HTTP response contains all the requested dynamic modules in
   a single JSON payload, gzip compression works across modules, which
   produces a smaller total response size than if each individual module
   was compressed by itself.

4. Although HTTP requests have more latency than WebSocket messages, the
   ability to make HTTP requests begins much sooner than a WebSocket
   connection can be established, which more than makes up for the latency
   disadvantage.

5. HTTP requests are a little easier to inspect and debug in the dev tools
   than WebSocket frames.

6. The new /__dynamicImport HTTP endpoint is a raw Connect/Express-style
   endpoint, so it bypasses the Meteor method-calling system altogether,
   which eliminates some additional overhead.

7. Fetching dynamic modules no longer competes with other WebSocket
   traffic such as DDP and Livedata.

8. Although the implementation of the /__dynamicImport endpoint is a bit
   too complicated to allow serving dynamic modules from a CDN, that
   remains a possibility for future experimentation. In other words, how
   modules are fetched is still just an implementation detail of the
   `meteorInstall.fetch` callback.

9. As with the WebSocket implementation, the module server is totally
   stateless, so it should be easy to scale up if necessary.

I wish I had appreciated the advantages of an HTTP-based implementation
sooner, but I think the transition will be pretty seamless, since the new
implementation is completely backwards compatible with the old.
Allow only server-side code to fetch dynamic server modules.
Each time the server starts, the dynamic-import/server.js module creates a
secret key for accessing dynamic server modules, then exposes that key to
the server-side dynamic-import/client.js module, and no one else.

If client.setSecretKey has been called, that key will be included as a
query parameter in each /__dynamicImport POST request. If it matches the
actual secret key, access is granted to the corresponding dynamic modules;
otherwise, only web.browser dynamic modules are made available.
Remove webapp and random dependencies from dynamic-import.
Letting dynamic-import depend on packages that depend on ecmascript means
ecmascript and its dependencies can't depend on dynamic-import. This
commit gives us back that flexibility.
Upgrade `install` to allow batching of dynamic import() requests.
Now that we're using HTTP POST requests to fetch dynamic modules, it's
more important to make fewer requests when possible, given the higher
latency of HTTP requests compared to WebSocket messages.

The trick is to wait until the next tick of the event loop before actually
sending the request, so that multiple dynamic import() calls in quick
succession are treated as a single request, and all the modules they
require can be returned in a single response object.

For example, we want code like this

  const [
    React,
    ReactDOM
  ] = await Promise.all([
    import("react"),
    import("react-dom")
  ]);

to result in one HTTP POST request for both `react` and `react-dom`, as
well as all their dependencies, rather than two separate requests.
Indeed, that is what happens, since both import() calls take place in the
same tick of the event loop.

@benjamn benjamn added this to the Release 1.6.1 milestone Nov 17, 2017

@benjamn benjamn self-assigned this Nov 17, 2017

@benjamn benjamn requested review from hwillson and abernix Nov 17, 2017

benjamn added some commits Nov 17, 2017

@hwillson

This is super exciting @benjamn - everything looks great!

I've never really noticed how similar the httpcall_client.js and httpcall_server.js code is (when it comes to things like parameter verification, callback wrapping, setting headers, etc.). At some point it might be worth moving that shared code up into httpcall_common.js, but I think that's outside the scope of this PR.

@@ -1,3 +0,0 @@
// The HTTP object used to be called Meteor.http.
// XXX COMPAT WITH 0.6.4

This comment has been minimized.

@hwillson

hwillson Nov 17, 2017

Member

I guess we've supported this long enough ... 🙂

This comment has been minimized.

@benjamn

benjamn Nov 17, 2017

Member

Think it's sufficient to mention this in History.md?

This comment has been minimized.

@hwillson

hwillson Nov 17, 2017

Member

Yes, I think so - people have had long enough to plan for this one being deprecated.

This comment has been minimized.

@abernix

abernix Nov 17, 2017

Member

It was mentioned in the 0.6.5 History.md! 😉

'url',
'ecmascript'
// This package intentionally does not depend on ecmascript, so that

This comment has been minimized.

@hwillson

hwillson Nov 17, 2017

Member

Thanks for adding this comment - I was curious about the switch back to pre-ES2015 syntax, so this definitely helped clarify things.

});
}
function getPlatform(request) {

This comment has been minimized.

@hwillson

hwillson Nov 17, 2017

Member

Some of the changes in this file revolve around the fact that a configurable platform is now supported. Should we add an extra comment somewhere explaining why? Maybe explaining this is better suited elsewhere (like the package README or API docs), but I just thought I'd mention it.

This comment has been minimized.

@benjamn

benjamn Nov 17, 2017

Member

We've always had web.browser and server as separate platforms, it's just that we were able to determine which one to use based on this.connection in the method implementation before, and now we have to detect the platform differently. The detection is still fully automatic, so I don't know that it needs to be advertised as a public API. More implementation comments could be nice, though!

Side note: I suppose web.cordova could also appear here, though we intentionally force dynamic modules into the main JS bundle for Cordova, because we don't have to keep re-downloading the bundle (so it's ok if it's bigger), and we can't necessarily eval dynamic code at runtime in some Cordova environments, due to security restrictions.

This comment has been minimized.

@hwillson

hwillson Nov 17, 2017

Member

Make sense @benjamn - looks good as it is.

summary: "CommonJS module system",
git: "https://github.com/benjamn/install",
documentation: "README.md"
});
Npm.depends({
install: "0.10.1"
install: "0.10.2"

This comment has been minimized.

@benjamn

benjamn Nov 17, 2017

Member

Here's the commit that implements module.prefetch batching: benjamn/install@b8e62ae

Move dynamic-import dependency from meteor-base to ecmascript.
Now that dynamic-import no longer depends indirectly on ecmascript, the
ecmascript package can finally guarantee support for dynamic `import()`,
as it rightfully should.
@abernix

Looks excellent.

return walk(tree);
function walk(node) {
if (node && typeof node === "object") {
let empty = true;

This comment has been minimized.

@abernix

abernix Nov 17, 2017

Member

The number of lines inside this if conditional has grown a bit with this commit. It could improve readability, and get rid of the cliff if the else and its return were first:

function walk(node) {
  if (typeof node !== "object") {
    return read(pathParts, platform);
  }

  let empty = true;
  // ... so on.
  return node;
}
@@ -1,3 +0,0 @@
// The HTTP object used to be called Meteor.http.
// XXX COMPAT WITH 0.6.4

This comment has been minimized.

@abernix

abernix Nov 17, 2017

Member

It was mentioned in the 0.6.5 History.md! 😉

@abernix

This comment has been minimized.

Member

abernix commented Nov 17, 2017

Though I guess the package test failures might be relevant!

Make bundle-visualizer use HTTP instead of Meteor.call.
If you're trying to visualize the bundle of an application that does not
use ddp-client, it's annoying if bundle-visualizer pulls in all those
dependencies just to support itself.

cc @abernix
@abernix

This comment has been minimized.

Member

abernix commented on d0566bb Nov 17, 2017

Awesome!

benjamn added some commits Nov 17, 2017

Remove weak dependency on browser-policy-common from dynamic-import.
Even a weak dependency affects the load order of packages by forcing the
depended-on package to load before the dependent package. Waiting until
Meteor.startup to check for Package["browser-policy-common"] dynamically
does not have this problem.

@benjamn benjamn merged commit 981fb3c into devel Nov 17, 2017

13 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Group 7 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jamiter

This comment has been minimized.

Contributor

jamiter commented on packages/http/httpcall_client.js in 2127501 Nov 18, 2017

Is there a reason you don’t use Object.assign for this? Or do you need the ecmascript package for that?

This comment has been minimized.

Member

benjamn replied Nov 19, 2017

Yes, specifically we need ecmascript-runtime, whereas Object.keys and Array.prototype.forEach are guaranteed by the es5-shim polyfill.

benjamn added a commit that referenced this pull request Dec 3, 2017

Try loading SockJS dynamically after native WebSocket errors.
To save size in modern browsers, JavaScript bundles built for the
web.browser architecture no longer statically include the SockJS library.
That's safe as long as native WebSockets actually work, but what if
there's a problem with the network that necessitates falling back to
long-polling or some other SockJS strategy?

In those cases, we can load SockJS using a dynamic import(), which is a
little slower than including it in the bundle, but that's OK because the
module will be permanently cached in IndexedDB in production, and falling
back to SockJS should be rare in modern browsers anyway.

Note that this trick would not be possible if the implementation of
dynamic import() still required a socket connection! (#9384)

benjamn added a commit that referenced this pull request Dec 4, 2017

Try loading SockJS dynamically after native WebSocket errors.
To save size in modern browsers, JavaScript bundles built for the
web.browser architecture no longer statically include the SockJS library.
That's safe as long as native WebSockets actually work, but what if
there's a problem with the network that necessitates falling back to
long-polling or some other SockJS strategy?

In those cases, we can load SockJS using a dynamic import(), which is a
little slower than including it in the bundle, but that's OK because the
module will be permanently cached in IndexedDB in production, and falling
back to SockJS should be rare in modern browsers anyway.

Note that this trick would not be possible if the implementation of
dynamic import() still required a socket connection! (#9384)

benjamn added a commit that referenced this pull request Dec 5, 2017

Try loading SockJS dynamically after native WebSocket errors.
To save size in modern browsers, JavaScript bundles built for the
web.browser architecture no longer statically include the SockJS library.
That's safe as long as native WebSockets actually work, but what if
there's a problem with the network that necessitates falling back to
long-polling or some other SockJS strategy?

In those cases, we can load SockJS using a dynamic import(), which is a
little slower than including it in the bundle, but that's OK because the
module will be permanently cached in IndexedDB in production, and falling
back to SockJS should be rare in modern browsers anyway.

Note that this trick would not be possible if the implementation of
dynamic import() still required a socket connection! (#9384)

benjamn added a commit that referenced this pull request Jan 3, 2018

Try loading SockJS dynamically after native WebSocket errors.
This is a back-port of a similar change on the web.browser.legacy branch:
b8601d3

To save size in modern browsers, JavaScript bundles built for the
web.browser architecture no longer statically include the SockJS library.
That's safe as long as native WebSockets actually work, but what if
there's a problem with the network that necessitates falling back to
long-polling or some other SockJS strategy?

In those cases, we can load SockJS using a dynamic import(), which is a
little slower than including it in the bundle, but that's OK because the
module will be permanently cached in IndexedDB in production, and falling
back to SockJS should be rare in modern browsers anyway.

Note that this trick would not be possible if the implementation of
dynamic import() still required a socket connection! (#9384)

benjamn added a commit that referenced this pull request Jan 22, 2018

Try loading SockJS dynamically after native WebSocket errors.
To save size in modern browsers, JavaScript bundles built for the
web.browser architecture no longer statically include the SockJS library.
That's safe as long as native WebSockets actually work, but what if
there's a problem with the network that necessitates falling back to
long-polling or some other SockJS strategy?

In those cases, we can load SockJS using a dynamic import(), which is a
little slower than including it in the bundle, but that's OK because the
module will be permanently cached in IndexedDB in production, and falling
back to SockJS should be rare in modern browsers anyway.

Note that this trick would not be possible if the implementation of
dynamic import() still required a socket connection! (#9384)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment