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

Make it a noop in Ember > 3.0.0-beta.2 #20

Merged
merged 2 commits into from
Jan 11, 2018

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Jan 11, 2018

Closes #18

Would you also output a console warning asking users to just remove the addon?
Also, should it be gt('3.0.0-beta.2'); or gte('3.0.0-beta.2');?

@rwjblue
Copy link
Owner

rwjblue commented Jan 11, 2018

should it be gt('3.0.0-beta.2'); or gte('3.0.0-beta.2');?

I think gte would work, but I can't recall if ember-cli-version-checker exposes a .gte 😝

@rwjblue
Copy link
Owner

rwjblue commented Jan 11, 2018

Would you also output a console warning asking users to just remove the addon?

This is what I have done in the past, but it makes annoying warnings for addons during ember-try scenarios.

Maybe we can tweak the logic to be:

    this._shouldDisable = true;
    if (emberVersion.lt('3.0.0-beta.2')) {
      this._shouldDisable = false;
    } else if (this.parent === this.project && !process.env.EMBER_TRY_CURRENT_SCENARIO){
      this.ui.writeWarnLine('ember-native-dom-event-dispatcher is not required for Ember 3.0.0-beta.2 and later, please remove from your `package.json`.');
    }

Adding the check for EMBER_TRY_CURRENT_SCENARIO should alleviate some of @Turbo87's prior concerns (RE: ember-polyfills/ember-getowner-polyfill#16)...

@cibernox
Copy link
Contributor Author

Done. It works pretty well and only outputs once.

@rwjblue
Copy link
Owner

rwjblue commented Jan 11, 2018

Merging this, would you mind sending another PR updating the README to explain that this addon is not needed with Ember 3.0?

@rwjblue rwjblue merged commit 6a0c3c0 into rwjblue:master Jan 11, 2018
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.

2 participants