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] fail builds in ember-cli when ember-cli-shims isn't met #4057

Merged
merged 1 commit into from
Jan 12, 2016

Conversation

fivetanley
Copy link
Member

fixes #4044

I disagree with @rwjblue that this should be a silent error. I've found this same kind of error to be nice to see which plugin is throwing an error where, in case I need to debug it or a file an issue about it.

@fivetanley
Copy link
Member Author

These fail in node because:

Using a version of ember-cli-shims prior to 0.1.0 will cause errors while loading Ember Data 2.3+. Please update ember-cli-shims from undefined to 0.1.0.
Error: Using a version of ember-cli-shims prior to 0.1.0 will cause errors while loading Ember Data 2.3+. Please update ember-cli-shims from undefined to 0.1.0.
    at Class.module.exports.init (/Users/stanley/code/data/index.js:47:13)
    at new Class (/Users/stanley/code/data/node_modules/core-object/core-object.js:18:12)
    at /Users/stanley/code/data/node_modules/ember-cli/lib/models/addons-factory.js:48:19
    at visit (/Users/stanley/code/data/node_modules/ember-cli/lib/utilities/DAG.js:23:3)
    at DAG.topsort (/Users/stanley/code/data/node_modules/ember-cli/lib/utilities/DAG.js:82:7)
    at AddonsFactory.initializeAddons (/Users/stanley/code/data/node_modules/ember-cli/lib/models/addons-factory.js:44:9)
    at Project.initializeAddons (/Users/stanley/code/data/node_modules/ember-cli/lib/models/project.js:375:36)
    at Project.eachAddonCommand (/Users/stanley/code/data/node_modules/ember-cli/lib/models/project.js:426:10)
    at module.exports (/Users/stanley/code/data/node_modules/ember-cli/lib/cli/lookup-command.js:34:13)
    at CLI.<anonymous> (/Users/stanley/code/data/node_modules/ember-cli/lib/cli/cli.js:36:26)

This error happens when init is ran the second time, but not the first.

@fivetanley
Copy link
Member Author

The node tests are generating a new app which is running npm install, wonder if we can override the bower dependency somehow. Maybe something in this file can help us? https://github.com/ember-cli/ember-cli-blueprint-test-helpers/blob/master/lib/helpers/setup.js

@rwjblue
Copy link
Member

rwjblue commented Jan 12, 2016

Consumers of ember-data do not care about Ember Data's internal stack trace for this error. You are specifying in the message exactly who is throwing the error. The stack is just needlessly noisey and will drown out the actual message.

@rwjblue
Copy link
Member

rwjblue commented Jan 12, 2016

It seems to me that we need more tailored messages. I see at least three scenarios that all seem to deserve their own messaging:

  • when _forceBowerUsage is true AND the bower version of ember-data is ^2.3.0-beta.5 then you must have ember-cli-shims at >= 0.1.0. We should suggest to folks that ember-data@2.3.x should be consumed from npm instead of bower.json and that they should remove ember-data from bower.json (this is a warning, not an error condition).
  • when _forceBowerUsage is true AND the bower version of ember-data is < 2.3.0-beta.3 (or whenever the shims were introduced to the ember-data bower build) then you must have ember-cli-shims at < 0.1.0 (otherwise the various ember-data modules are not found).
  • when _forceBowerUsage is false AND ember-cli-shims is < 0.1.0. We should throw a SilentError with a message telling them that they must use ember-cli-shims@0.1.0.

@fivetanley
Copy link
Member Author

Consumers of ember-data do not care about Ember Data's internal stack trace for this error. You are specifying in the message exactly who is throwing the error. The stack is just needlessly noisey and will drown out the actual message.

In my testing, this dies very fast and the error message and stack are clear to see. It would be useful to have the stack later when people will inevitably open issues for this further down the line. If people choose to debug this themselves, having the stack is very useful. I needed it for Ember Pouch to try to get it to load for this version of Ember Data. I wouldn't have pursued it otherwise.

I see at least three scenarios that all seem to deserve their own messaging

I agree, seems great.

@rwjblue
Copy link
Member

rwjblue commented Jan 12, 2016

@fivetanley - Thanks for working on this. I'm sorry that I didn't think all of this through when I added the warning...

@fivetanley
Copy link
Member Author

I may skip the assertion when we're running node tests for now. There's not a way to get in the middle of the test helpers and install the correct version of ember-cli-shims before the test helpers run as far as I can tell.

@fivetanley fivetanley force-pushed the assert-ember-cli-shims branch 2 times, most recently from d4e24bb to dd3f3c6 Compare January 12, 2016 05:02
@fivetanley
Copy link
Member Author

Once travis comes back green we are good to go. I tested each of the scenarios Rob outlined in a new ember app.

fivetanley added a commit that referenced this pull request Jan 12, 2016
[BUGFIX beta] fail builds in ember-cli when ember-cli-shims isn't met
@fivetanley fivetanley merged commit deb3d7e into emberjs:master Jan 12, 2016
@fivetanley fivetanley deleted the assert-ember-cli-shims branch January 12, 2016 05:46
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.

If ember-cli-shims is not >= 0.1.0, this should cause the build to fail.
2 participants