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.

Sign up for GitHub

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

Jump to bottom

[DEPRECATION] #14754 deprecate canDispatchToEventManager #15078

Merged
merged 2 commits into from
Apr 3, 2017
Merged

[DEPRECATION] #14754 deprecate canDispatchToEventManager #15078

merged 2 commits into from
Apr 3, 2017

Conversation

habdelra
Copy link
Contributor

@habdelra habdelra commented Mar 27, 2017 •
edited
Loading

This is to address the deprecation in #14754. I'm not sure if the deprecation message provides enough detail.

@habdelra habdelra force-pushed the 14754-deprecate-canDispatchToEventManager branch 3 times, most recently from f470f89 to 4007121 Compare March 27, 2017 23:26
jpadilla
jpadilla reviewed Mar 27, 2017
View reviewed changes
packages/ember-glimmer/tests/integration/event-dispatcher-test.js Outdated

expectDeprecation(() => {
this.render(`{{x-foo}}`);
}, '[DEPERECATED] `canDispatchToEventManager` has been deprecated.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be [DEPRECATED] instead of [DEPERECATED].

mmun
mmun reviewed Mar 27, 2017
View reviewed changes
Copy link
Member

@mmun mmun left a comment

Choose a reason for hiding this comment

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

Thank you for participating in the contributor's workshop!! :)

packages/ember-glimmer/tests/integration/event-dispatcher-test.js Outdated
ComponentClass: Component.extend({
eventManager: {
myEvent() {
assert.ok(true, 'custom event was triggered');
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear from how this test is written that this line of code is actually executing. Can you set a boolean and check it at the end, or use assert.expect?

Copy link
Contributor Author

@habdelra habdelra Mar 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

whoops--thats copy-pasta, that assert is completely unrelated to the changes in this PR

@habdelra habdelra force-pushed the 14754-deprecate-canDispatchToEventManager branch from 4007121 to f36fcbe Compare March 28, 2017 00:03
@habdelra
Copy link
Contributor Author

@rwjblue could I get your

rwjblue
rwjblue requested changes Mar 28, 2017
View reviewed changes
packages/ember-glimmer/tests/integration/event-dispatcher-test.js Outdated
@@ -138,6 +139,34 @@ moduleFor('EventDispatcher#setup', class extends RenderingTest {
this.$('div').trigger('myevent');
}

['@test canDispatchToEventManager is deprecated'](assert) {
this.dispatcher.canDispatchToEventManager = null;
Copy link
Member

Choose a reason for hiding this comment

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

In theory, we should not need to force this to be null in this test, the default of undefined should be enough

packages/ember-views/lib/mixins/view_support.js Outdated
@@ -538,7 +538,16 @@ export default Mixin.create({
let owner = getOwner(this);
let dispatcher = owner && owner.lookup('event_dispatcher:main');

if (dispatcher && dispatcher.canDispatchToEventManager === null) {
deprecate(
`[DEPRECATED] \`canDispatchToEventManager\` has been deprecated.`,
Copy link
Member

Choose a reason for hiding this comment

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

Let's have this message mention eventManager instead of canDispatchToEventManager, and also add the ${this} to the interpolation (so it's clear to someone seeing the deprecation which component caused it.

packages/ember-views/lib/mixins/view_support.js Outdated
!('canDispatchToEventManager' in dispatcher),
{
id: 'ember-views.event-dispatcher.canDispatchToEventManager',
until: '3.0.0'
Copy link
Member

Choose a reason for hiding this comment

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

Let's change this to 2.16.0

packages/ember-views/lib/system/event_dispatcher.js Outdated

init() {
this._super();
assert('EventDispatcher should never be instantiated in fastboot mode. Please report this as an Ember bug.', environment.hasDOM);

deprecate(
`[DEPRECATED] \`canDispatchToEventManager\` has been deprecated.`,
Copy link
Member

Choose a reason for hiding this comment

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

Let's mention which file this is in.

Copy link
Member

Choose a reason for hiding this comment

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

Also, you should not need to prefix the message with [DEPRECATED]

habdelra pushed a commit to habdelra/ember.js that referenced this pull request Mar 28, 2017
Updating PR emberjs#15078 with rwjblue review feedback
523d8da
@habdelra habdelra force-pushed the 14754-deprecate-canDispatchToEventManager branch from 523d8da to 6810123 Compare March 28, 2017 15:20
@habdelra
Copy link
Contributor Author

@rwjblue good morning sunshine, I need more

@rwjblue rwjblue merged commit 77078af into emberjs:master Apr 3, 2017
@rwjblue
Copy link
Member

rwjblue commented Apr 3, 2017

Thank you @habdelra!

@mixonic mixonic mentioned this pull request Jun 27, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmun mmun mmun left review comments

@jpadilla jpadilla jpadilla left review comments

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@habdelra @rwjblue @jpadilla @mmun