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 release] Only setup babel options once. #4328

Merged
merged 1 commit into from
Apr 13, 2016

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Apr 13, 2016

included is called for each instance of EmberApp created within the consuming applicationm, which means that we would be adding the same plugins if the consuming application happened to be creating two EmberApp instances.

For example, ember-cli-fastboot currently uses this technique to build both for fastboot AND the normal build. I know of a few other applications that use similar techniques to build two versions of the same EmberApp (i.e. a mobile version and a desktop one).

This allows us to ensure that this.options.babel is only setup once (regardless of how many times included is being called).

Fixes #4322.

@fivetanley
Copy link
Member

We are working around this in ember-cli-fastboot by calling the function in ember-cli-build twice (creating two instances). This sort of feels like scenario solving; It feels like Ember CLI should have a better EmberApp cloning primitive so these kind of issues wouldn't happen. It also feels surprising as an addon author that things get called twice per instance.

@fivetanley
Copy link
Member

I'll still merge once travis passes as a workaround. It feels like something is missing from the addon author perspective though.

@rwjblue
Copy link
Member Author

rwjblue commented Apr 13, 2016

@fivetanley - I don't feel like it is scenario solving. We only need to setup our addon's options once, not once for each time it is included into something else. Every time included is called we add more items to the list of plugins. Ideally this can go into init (I will test that actually), because it is one time setup stuff...

@rwjblue rwjblue force-pushed the prevent-errors-with-fastboot branch from 59552c5 to eaea053 Compare April 13, 2016 19:44
@rwjblue
Copy link
Member Author

rwjblue commented Apr 13, 2016

Yes, moving to init seems to work properly. Updated.

@bmac
Copy link
Member

bmac commented Apr 13, 2016

Looks like the build failed because a call to warnMessageForUndefinedType wasn't getting striped. Unfortunately I can't reproduce the error locally.

`included` is called for each instance of `EmberApp` created
within the consuming applicationm, which means that we would be adding
the same plugins if the consuming application happened to be creating
two `EmberApp` instances.

For example, ember-cli-fastboot currently uses this
technique to build both for fastboot AND the normal build. I know of a
few other applications that use similar techniques to build two versions
of the same `EmberApp` (i.e. a mobile version and a desktop one).

This allows us to ensure that `this.options.babel` is only setup once
(regardless of how many times `included` is being called) during init.
@rwjblue rwjblue force-pushed the prevent-errors-with-fastboot branch from eaea053 to 545f0a3 Compare April 13, 2016 22:01
@rwjblue
Copy link
Member Author

rwjblue commented Apr 13, 2016

OK, updated again. Should fix the error that we were hitting before. When I moved the setup into init, the process.env.EMBER_APP environment variable wasn't set yet, so things were not properly stripped. That is why we changed this to be done in included to begin with and this is pretty annoying. I'll start thinking about a better solution, but I do think that this is a good enough work around for now...

@fivetanley
Copy link
Member

r+

@fivetanley fivetanley merged commit 1c7c737 into emberjs:master Apr 13, 2016
@rwjblue rwjblue deleted the prevent-errors-with-fastboot branch April 13, 2016 22:33
@lmcardle
Copy link

Any thoughts for users on the best workaround for now? I also just ran into this problem.

@fivetanley
Copy link
Member

Can you tell us more about the situation you are running into this with? Typically it has been fastboot only as far as I can tell.

@fivetanley
Copy link
Member

Otherwise you can use this addon from master.

@lmcardle
Copy link

Yes it was fast boot in my case. I'll use master as you suggest. Thanks.

On Wednesday, April 13, 2016, Stanley Stuart [email protected]
wrote:

Otherwise you can use this addon from master.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#4328 (comment)

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.

4 participants