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

Add ability to deduplicate babel helpers. #251

Merged
merged 1 commit into from Dec 20, 2018

Conversation

Projects
None yet
4 participants
@pzuraq
Copy link
Contributor

pzuraq commented Nov 8, 2018

This PR sets up the groundwork for allowing Ember apps to externalize Babel helpers, reducing the amount of code shipped to the client by deduplicating common code included by Babel.

Externalized helpers represent a unique problem, because in order to truly deduplicate all helpers, the app and all of its addons need to have a shared contract for which helpers will be included in the global namespaces and which will not. Currently we don't have a method to coalesce configurations from many addons, so the current implementation only works with a single, global, app-wide configuration. Eventually this could be set to reasonable defaults, but now it is left off.

Still WIP, definitely needs testing and tests.

@rwjblue
Copy link
Member

rwjblue left a comment

Some fundamental issues here:

  • How do we deal with helper incompatibility between versions (both major versions and minor/patch)?
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated

@pzuraq pzuraq force-pushed the pzuraq:feat/externalize-helpers-by-default branch from eb0b90d to 95283a0 Nov 9, 2018

@pzuraq pzuraq changed the title [WIP][FEAT] Adds externalized helpers option [FEAT] Adds externalized helpers option Nov 9, 2018

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Nov 9, 2018

Alright, I think this is ready for another round of review. Added tests and made sure everything actually working the way it should be!

@pzuraq pzuraq force-pushed the pzuraq:feat/externalize-helpers-by-default branch 2 times, most recently from aa695e2 to 468c45a Nov 9, 2018

@pzuraq pzuraq force-pushed the pzuraq:feat/externalize-helpers-by-default branch from 468c45a to 9291be1 Dec 18, 2018

Show resolved Hide resolved README.md
Show resolved Hide resolved README.md
Show resolved Hide resolved README.md
Show resolved Hide resolved README.md

@pzuraq pzuraq force-pushed the pzuraq:feat/externalize-helpers-by-default branch from 6eddb03 to 8140161 Dec 18, 2018

index.js Outdated
@@ -123,6 +191,12 @@ module.exports = {
return (this.parent && this.parent.options) || (this.app && this.app.options) || {};
},

_getHostOptions() {
let host = this._findHost();

This comment has been minimized.

@dfreeman

dfreeman Dec 19, 2018

Contributor

The EngineAddon wrapper in ember-engines overrides _findHost to point to the first lazy-loading engine it comes to rather than the root app. We may need to explicitly traverse the parent chain ourselves here.

This comment has been minimized.

@dfreeman

dfreeman Dec 19, 2018

Contributor

Actually, since this only ever needs the host options when this.parent === this.project, I think we can just access this.app directly and not need to bother with findHost.

[
require.resolve('@babel/plugin-transform-runtime'),
{
version: this._getHelperVersion(),

This comment has been minimized.

@dfreeman

dfreeman Dec 19, 2018

Contributor

I see how they're using version in the implementation, but I can't find any documentation for it on the plugin page. I don't imagine that's intended to be a "secret" option, but have you seen anything indicating it's public?

This comment has been minimized.

@pzuraq

pzuraq Dec 19, 2018

Author Contributor

I have not, but it's definitely needed to actually include decorator helpers or anything more recent than 7.0.0-0, so I can't imagine it's not public? Does seem very strange to me though

@pzuraq pzuraq force-pushed the pzuraq:feat/externalize-helpers-by-default branch 4 times, most recently from 1be3132 to 723a038 Dec 20, 2018

[FEAT] Adds an option to emit external babel helpers
This PR adds the `includeExternalHelpers` config option which allows
users to explicitly opt into using externalized helpers for transpiled
code. In large apps this can help with app size, especially in apps
which are making heavy usage of new ES features like native classes and
decorators.

This option is a _global_ option, meaning it _must_ be configured in the
root application. The reason for this is because there must be one
canonical source of truth for which version of helpers are being used,
and whether or not helpers are being used at all. If one addon decides
to use helpers, the app must include them, and they must be the right
version.

By default, ec-babel will check the dependency tree to see if there are
any addons which exist which will incur heavy expenses, such as
decorators. If so, it'll externalize helpers by default. Eventually this
behavior will be unnecessary due to treeshaking.

The current setup will _only_ support babel 7 helpers. This can be
expanded upon in the future, but the complexity of doing so makes it
beyond the scope of the initial work here.

@pzuraq pzuraq force-pushed the pzuraq:feat/externalize-helpers-by-default branch from 723a038 to d8c2b1b Dec 20, 2018

@rwjblue rwjblue added the enhancement label Dec 20, 2018

@rwjblue rwjblue changed the title [FEAT] Adds externalized helpers option Add ability to deduplicate babel helpers. Dec 20, 2018

@rwjblue rwjblue merged commit ff09b76 into babel:master Dec 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pzuraq pzuraq deleted the pzuraq:feat/externalize-helpers-by-default branch Dec 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment