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

[BUGFIX beta][FEAT] Adds a second dist build which targets IE #17188

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Nov 7, 2018

This PR adds a second dist to the build which targets IE. This allows
the primary bundles to drop ES5 compilation from babel, and allows us to
choose which bundles we want to use based on the consuming application's
targets.

In the long run, we want to actually enable consuming apps to build the
Ember source code themselves. This is a temporary in-between measure to
address a bug in native classes, in which users who do not have IE as
a build target cannot .extend from native classes due to conflicts
in transpilation vs actual native class syntax.

ember-cli-build.js Outdated Show resolved Hide resolved
@pzuraq pzuraq force-pushed the bugfix/ie-dual-build branch from c60888e to cdf2ff0 Compare November 8, 2018 00:52
bin/run-tests.js Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be checked in (maybe we should add it to .gitignore?)

ember-cli-build.js Outdated Show resolved Hide resolved
ember-cli-build.js Outdated Show resolved Hide resolved
@rwjblue
Copy link
Member

rwjblue commented Nov 8, 2018

Oh, also can you update the commit message to start with [BUGFIX beta]?

@pzuraq pzuraq force-pushed the bugfix/ie-dual-build branch from cdf2ff0 to e2d98ae Compare November 9, 2018 06:54
@pzuraq pzuraq changed the title [BUGFIX][FEAT] Adds a second dist build which targets IE [BUGFIX beta][FEAT] Adds a second dist build which targets IE Nov 9, 2018
@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 9, 2018

ALrighty, restructured to a better flow:

  1. Gather all files
  2. Transpile to ES5
  3. Debug macros + strip and prep for production
  4. Gather files into bundles
  5. Transform AMD and concat files

This flow ended up making the most sense to me given that we must do
the AMD transform last. It keeps most major operations at a per-file
level, and saves the file-divying and concatting for last.

Anecdotally, the restructure does seem to be a bit slower, and there are some inconsistencies in the final builds. Specifically, ember.prod.js seems to contain more comments and have less code removed by the minifier. ember.min.js seems to be mostly unaffected:

# before
 - dist/ember-template-compiler.js: 404.07 KB (80.96 KB gzipped)
 - dist/ember-testing.js: 78.35 KB (17.96 KB gzipped)
 - dist/ember-tests.js: 3.15 MB (417.24 KB gzipped)
 - dist/ember-tests.prod.js: 1.06 MB (141.87 KB gzipped)
 - dist/ember.debug.js: 1.74 MB (357.91 KB gzipped)
 - dist/ember.min.js: 469.15 KB (120.22 KB gzipped)
 - dist/ember.prod.js: 1.6 MB (329.29 KB gzipped)

# after
 - dist/ie/ember-template-compiler.js: 408.98 KB (81.99 KB gzipped)
 - dist/ie/ember-testing.js: 78.93 KB (17.95 KB gzipped)
 - dist/ie/ember-tests.js: 3.17 MB (420.78 KB gzipped)
 - dist/ie/ember-tests.prod.js: 1.07 MB (143.26 KB gzipped)
 - dist/ie/ember.debug.js: 1.91 MB (399.59 KB gzipped)
 - dist/ie/ember.min.js: 469.38 KB (120.36 KB gzipped)
 - dist/ie/ember.prod.js: 1.77 MB (371.62 KB gzipped)

@rwjblue
Copy link
Member

rwjblue commented Nov 9, 2018

Hmm, I would not have expected the sizes to differ so much, would you mind attaching a .tar.gz of the dist/ (before and after) so I can poke through it?

@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 9, 2018

Sure thing!

dist-dual-build.tar.gz
dist-master.tar.gz

@pzuraq pzuraq force-pushed the bugfix/ie-dual-build branch from e2d98ae to 5aec3c4 Compare November 9, 2018 15:48
@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 14, 2018

I've been diffing the output bundles and while I haven't checked every single difference, a lot of them seem to come to a few categories:

  1. Comments are not being removed from the final output, when they previously were. Comments are still removed from the final min build, so this isn't really a concern for that build and in fact they probably should not have been removed in the other builds.

  2. Dead code elimination isn't quite working in the ember.prod.js build. Specifically, it appears to do a single pass and remove tautologies, but then does not remove functions which were only used in those tautological branches. This again does not affect the min build (verified by reading the actual function code).

  3. Very small differences in the way transpilation/minification occurs. For instance, in one build an extra pair of enclosing parens may be emitted around an expression. In another, a variable which was inlined was no longer inlined.

Surprisingly, removing the minify-dead-code-elimination plugin actually reduces the final build size to below the original final build, so it seems like the plugin may have been interfering with the minification process more than helping.

# before
 - dist/ie/ember-template-compiler.js: 408.98 KB (81.99 KB gzipped)
 - dist/ie/ember-testing.js: 78.93 KB (17.95 KB gzipped)
 - dist/ie/ember-tests.js: 3.17 MB (420.78 KB gzipped)
 - dist/ie/ember-tests.prod.js: 1.07 MB (143.26 KB gzipped)
 - dist/ie/ember.debug.js: 1.91 MB (399.59 KB gzipped)
 - dist/ie/ember.min.js: 469.38 KB (120.36 KB gzipped)
 - dist/ie/ember.prod.js: 1.77 MB (371.62 KB gzipped)

# after
 - dist/ie/ember-template-compiler.js: 417.36 KB (83 KB gzipped)
 - dist/ie/ember-testing.js: 78.93 KB (17.95 KB gzipped)
 - dist/ie/ember-tests.js: 3.17 MB (420.78 KB gzipped)
 - dist/ie/ember-tests.prod.js: 1.11 MB (154.61 KB gzipped)
 - dist/ie/ember.debug.js: 1.91 MB (399.59 KB gzipped)
 - dist/ie/ember.min.js: 470.61 KB (120.07 KB gzipped)
 - dist/ie/ember.prod.js: 1.84 MB (386.31 KB gzipped)

# non-ie build
 - dist/ember-template-compiler.js: 385.2 KB (79.08 KB gzipped)
 - dist/ember-testing.js: 76.5 KB (17.55 KB gzipped)
 - dist/ember-tests.js: 2.54 MB (340.77 KB gzipped)
 - dist/ember-tests.prod.js: 954.85 KB (130.86 KB gzipped)
 - dist/ember.debug.js: 1.75 MB (378.37 KB gzipped)
 - dist/ember.min.js: 407.55 KB (113.57 KB gzipped)
 - dist/ember.prod.js: 1.71 MB (367.31 KB gzipped)

@pzuraq pzuraq force-pushed the bugfix/ie-dual-build branch 2 times, most recently from ec14cb9 to df153d7 Compare November 15, 2018 22:15
This PR adds a second dist to the build which targets IE. This allows
the primary bundles to drop ES5 compilation from babel, and allows us to
choose which bundles we want to use based on the consuming application's
targets.

In the long run, we want to actually enable consuming apps to build the
Ember source code themselves. This is a temporary in-between measure to
address a bug in native classes, in which users who do _not_ have IE as
a build target cannot `.extend` from native classes due to conflicts
in transpilation vs actual native class syntax.
@pzuraq pzuraq force-pushed the bugfix/ie-dual-build branch from df153d7 to 02af025 Compare November 19, 2018 18:22
@rwjblue rwjblue merged commit 620f97c into emberjs:master Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants