-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Fix remaining ember-source issues. #6484
Conversation
@@ -335,7 +335,7 @@ EmberApp.prototype._initVendorFiles = function() { | |||
} | |||
|
|||
// don't crash if ember-cli-shims is not included | |||
if (!bowerDeps['ember-cli-shims']) { | |||
if (!ember && !bowerDeps['ember-cli-shims']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems somewhat odd. The idea here was that a different addon could provide the shims. That would work even with using Ember from bower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ember-source
build provides shims
every time.
@martndemus this was introduced in #6254. What is the story behind this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, what's the story of this: #6254 (comment)
It's distinct from the ember-resolver
issue but in that PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What @rwjblue said, to be able to provide my own shims addon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually rather opposed to this check existing as it makes the failure for not including shims move to runtime where we can't control it as easily and manifests as:
cannot find module 'ember'
Regardless, knowing the story makes it possible for me to address. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so the only thing which requires this change is ember-new-modules-shim
. The users of that addon should be assumed to be highly familiar Ember devs.
Given that, if users of ember-new-modules-shim
wish to not include ember-cli-shims
they will need to follow the full set of instructions at https://github.com/DockYard/ember-new-modules-shim#optional-disabling-ember-cli-shims ... not just remove the bower
dependency and rely on this change.
That is the only way we can make sure that shims are not included across both ember-source
-provided and bower
-provided shims while knowing user intent.
…its place. Change introduced here: ember-cli#6254 (comment) Discussion here: ember-cli#6484 (comment)
dd2fa3a
to
3221763
Compare
👍 , seems good |
Skipping @homu because CI is broken. |
The removal from
bower
meant that we were erroneously removing it fromember-source
. No longer.