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

[ENHANCEMENT] Add podModulePrefix deprecation for generate and destroy commands #3546

Merged
merged 4 commits into from
Mar 17, 2015

Conversation

trabus
Copy link
Contributor

@trabus trabus commented Mar 16, 2015

This PR fulfills #3424 by adding deprecation warnings for generate and destroy commands, as well as bumping the ember-resolver version to 0.1.14.

The deprecation warnings were not using the ui, so I added an additional version of deprecate that allows you to pass the ui in. I also changed cli.run() so it returns an object containing the ui so we can now check the ui inside of acceptance tests. So now inside any ember-cli acceptance test you need to check the ui output by doing the following:

it('outputs ui text', function() {
  ember(['new','my-app']).then(function(result){
    expect(result.ui.output).to.include('new');
  });
});

@trabus trabus changed the title Added podModulePrefix deprecation for generate and destroy commands [BREAKING ENHANCEMENT] Added podModulePrefix deprecation for generate and destroy commands Mar 16, 2015
@trabus trabus changed the title [BREAKING ENHANCEMENT] Added podModulePrefix deprecation for generate and destroy commands [BREAKING ENHANCEMENT] Add podModulePrefix deprecation for generate and destroy commands Mar 16, 2015
@@ -4,7 +4,7 @@
"jquery": "^1.11.1",
"ember": "1.10.0",
"ember-data": "1.0.0-beta.15",
"ember-resolver": "~0.1.12",
"ember-resolver": "~0.1.13",
Copy link
Member

Choose a reason for hiding this comment

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

Had to release 0.1.14 to fix an issue, can you bump to that?

@rwjblue
Copy link
Member

rwjblue commented Mar 16, 2015

The message says breaking, but this doesn't seem to actually break anything (but rather it deprecates and keeps everything functional). Am I mis-interpreting something?

@trabus
Copy link
Contributor Author

trabus commented Mar 16, 2015

@rwjblue You're right, I'll remove 'breaking'. I was more going for the fact that people will need to change their setups if they're using podModulePrefix to not get the warning, so I was counting that as breaking.

@trabus trabus changed the title [BREAKING ENHANCEMENT] Add podModulePrefix deprecation for generate and destroy commands [ENHANCEMENT] Add podModulePrefix deprecation for generate and destroy commands Mar 16, 2015
@@ -86,14 +86,21 @@ CLI.prototype.run = function(environment) {
// Possibly this issue: https://github.com/joyent/node/issues/8329
// Wait to resolve promise when running on windows.
// This ensures that stdout is flushed so acceptance tests get full output
var result = exitCode;
if (this.testing) {
result = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue @stefanpenner I just wanted to make sure this was okay before you merge, it doesn't seem to be breaking anything by returning this object instead of just the exitCode.

@rwjblue
Copy link
Member

rwjblue commented Mar 16, 2015

LGTM

@trabus
Copy link
Contributor Author

trabus commented Mar 16, 2015

Ugh, random AppVeyor timeout, I don't know why it hates me so.

@rwjblue
Copy link
Member

rwjblue commented Mar 16, 2015

restarted build

@trabus
Copy link
Contributor Author

trabus commented Mar 16, 2015

Looks like more random timeouts.

@rwjblue
Copy link
Member

rwjblue commented Mar 16, 2015

LGTM

@stefanpenner - r?

@@ -27,5 +27,7 @@ resolve('ember-cli', {
cliArgs: process.argv.slice(2),
inputStream: process.stdin,
outputStream: process.stdout
}).then(exit);
}).then(function(result) {
exit(result && result.exitCode || result);
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think we need to handle falsey exit codes 0 is an exit code and is falsey.

Something like:

var exitCode = typeof result === 'object' ? result.exitCode : result;

exit(exitCode);

@@ -27,5 +27,8 @@ resolve('ember-cli', {
cliArgs: process.argv.slice(2),
inputStream: process.stdin,
outputStream: process.stdout
}).then(exit);
}).then(function(result) {
var exitCode = typeof result === 'object' ? result.exitCode : result;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm nervous to make changes in this file, as it is shared between multiple versions of ember-cli and lasts a long time, can we avoid it?

Copy link
Member

Choose a reason for hiding this comment

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

This file's change should be perfectly backwards compatible (we allow for result to either be an object (new thing) or a number (old thing)). We should add a comment saying why we are doing the ternary (and what version it was added in so we know when we can remove it).

@stefanpenner - In theory we could avoid changing this file, but then we could not test the UI output in acceptance tests...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was too, but providing access to the ui for testing is definitely worth it IMO. This change essentially makes returning an object possible while still returning the exitCode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll gladly add a comment explaining what this does and why, with reference to cli.js

Copy link
Contributor

Choose a reason for hiding this comment

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

I was too, but providing access to the ui for testing is definitely worth it IMO. This change essentially makes returning an object possible while still returning the exitCode.

I believe this is in-fact worth it. I just wanted to be confident that this was thoroughly taken into account. I am happy with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefanpenner did you want me to put in the comment, or is this good as-is?

Copy link
Contributor

Choose a reason for hiding this comment

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

good as-is. If at some-point someone whats to add this to the architecture.md that would be lovely.

@trabus
Copy link
Contributor Author

trabus commented Mar 17, 2015

@rwjblue @stefanpenner I think this is ready to go. Let me know if you want anything else done on it.

rwjblue added a commit that referenced this pull request Mar 17, 2015
[ENHANCEMENT] Add podModulePrefix deprecation for generate and destroy commands
@rwjblue rwjblue merged commit bd43474 into ember-cli:master Mar 17, 2015
@trabus
Copy link
Contributor Author

trabus commented Mar 17, 2015

@rwjblue I'll get a PR going to reflect this in the docs to go with the next release. When are we planning to release 0.2.1?

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.

None yet

4 participants