Skip to content
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

Selectively server-render es5-shim <script> tag, as with sockjs-shim. #9360

Merged
merged 4 commits into from Nov 14, 2017

Conversation

@benjamn
Copy link
Member

@benjamn benjamn commented Nov 13, 2017

Just as we were able to remove the SockJS polyfill library entirely from the client JS bundle for modern browsers (#9353), we can do the same with the es5-shim package.

The removes about 28KB from the size of the minified JS bundle (before gzip).

It is no longer necessary for other packages to register weak dependencies on es5-shim, because es5-shim will be reliably loaded in the <head> if installed, which guarantees it will be evaluated before any bundled JS in packages or the application.

In addition to loading es5-shim and es5-sham using <script> tags on the client, we no longer evaluate either script on the server, since Node 8 has solid support for everything that previously needed shimming.

@benjamn benjamn added this to the Release 1.6.1 milestone Nov 13, 2017
@benjamn benjamn self-assigned this Nov 13, 2017
@benjamn benjamn requested review from hwillson and abernix Nov 13, 2017
@benjamn benjamn force-pushed the server-render-es5-shim branch from 01c1417 to de6084e Nov 13, 2017
@benjamn benjamn changed the title Server-render es5-shim <script> tags like sockjs-shim. Selectively server-render es5-shim <script> tag, as with sockjs-shim. Nov 14, 2017
Copy link
Member

@hwillson hwillson left a comment

Looks great @benjamn! There appears to be quite a bit in common between es5-shim/server.js and sockjs-shim/server.js, so there might be a chance to DRY things up a bit. Since they're in different packages though maybe it's not worth the effort, but perhaps we could have a new shim-common package that both packages reference, with something like the following in its own common.js:

const hasOwn = Object.prototype.hasOwnProperty;

export function doNotNeedShim(request, minimumMajorVersions, queryParam) {
  const { browser, url } = request;
  const query = url && url.query;
  const forceEs5Shim = query && query[queryParam];
  if (! forceEs5Shim &&
      browser &&
      hasOwn.call(minimumMajorVersions, browser.name) &&
      browser.major >= minimumMajorVersions[browser.name]) {
    return true;
  }
  return false;
}

export function makeScript(packagePath) {
  return `\n<script src="/packages/${packagePath}` +
    `${Meteor.isProduction ? '.min' : ''}.js"></script>`;
}

Again, might not be worth the effort - 🍔 for thought anyways.

@benjamn
Copy link
Member Author

@benjamn benjamn commented Nov 14, 2017

That's a totally valid point @hwillson, and I like the surface area of shared code that you've suggested.

@benjamn benjamn force-pushed the server-render-es5-shim branch from e38aa34 to 7da5d83 Nov 14, 2017
benjamn added 4 commits Nov 14, 2017
Just as we were able to remove the SockJS polyfill library entirely from
the client JS bundle for modern browsers (#9353), we can do the same with
the `es5-shim` package.

The removes 28KB from the size of the minified JS bundle (before gzip).

It is no longer necessary for other packages to register weak dependencies
on `es5-shim`, because `es5-shim` will be reliably loaded in the `<head>`
if installed, which guarantees it will be evaluated before any bundled JS
in packages or the application.

In addition to loading `es5-shim` and `es5-sham` using `<script>` tags on
the client, we no longer evaluate either script on the server, since Node
8 has solid support for everything that previously needed shimming.
Now that es5-shim has no impact on modern browsers (thanks to selectively
server-side rendering a <script> tag for older browsers), there's very
little reason not to use it.

If you really don't want to use it, you can always remove meteor-base and
install your preferred set of base packages.
@benjamn benjamn force-pushed the server-render-es5-shim branch from 5fd3905 to d230ad5 Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants