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

Avoid modifier manager capabilities deprecation #13

Merged
merged 2 commits into from
Aug 23, 2019

Conversation

wongpeiyi
Copy link
Contributor

Not sure if this is desirable but I encountered deprecation implicit-modifier-manager-capabilities: https://github.com/emberjs/ember.js/blob/master/packages/%40ember/-internals/glimmer/lib/modifiers/custom.ts#L141-L148 recently added to master, so sought to address it

Created an intermediary exports file to support older versions of Ember where _modifierManagerCapabilities is not exported

@wongpeiyi wongpeiyi force-pushed the fix/capabilities branch 2 times, most recently from e594721 to e5c4c40 Compare August 20, 2019 16:38
Copy link
Member

@rwjblue rwjblue 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 doing this! I left a small suggestion / tweak (so we can use the official public APIs), but otherwise this is great!

@@ -1,4 +1,4 @@
import Ember from 'ember';
import { setModifierManager, capabilities } from '../modifier';
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind updating to [email protected] so we can remove this (and the underscored _setModifierManager usage over in addon/modifier.js) in favor of:

import { setModifierManager, capabilities } from '@ember/modifier';

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 I may be way off here, but it looks like that doesn't work prior to canary because of a typo introduced in 3.7 that was fixed in:
emberjs/ember.js@b8a5633#diff-c424149274ab88ce43f2847da12c5ce0L531

It was exported under the global as Ember._modifierManagerCapabilties (missing "i" before "ties") so I'm thinking the mapping doesn't work until that commit...? I'm not sure how to address this other than the intermediary file solution

Copy link
Member

Choose a reason for hiding this comment

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

😩 - Ya, good point

Addresses deprecation `implicit-modifier-manager-capabilities`:
Custom modifier managers must define their capabilities using the
capabilities() helper function
@sukima
Copy link

sukima commented Aug 20, 2019

I attempted to copy this PR to my addon ember-oo-modifiers and I keep getting an error:

 @ember/modifier does not have a capabilities export
  1 | import Ember from 'ember';
> 2 | import { setModifierManager, capabilities } from '@ember/modifier';
    | ^

I checked and my yarn resolves ember-modifier-manager-polyfill at 1.0.3 and yours is at 1.0.1. I really confused at this point. Also emberjs/ember.js@b8a5633#diff-d2eb8d61021970ff9cac80a6fdd336a9L1 still has a misspelling in it.

@josemarluedke
Copy link

@sukima I believe you need to make sure you have ember-cli-babel v7.10.0.

@@ -1,4 +1,4 @@
import Ember from 'ember';
import { setModifierManager, capabilities } from '../modifier';
Copy link
Member

Choose a reason for hiding this comment

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

😩 - Ya, good point

@rwjblue
Copy link
Member

rwjblue commented Aug 21, 2019

@sukima I believe you need to make sure you have ember-cli-babel v7.10.0.

confirm

@wongpeiyi
Copy link
Contributor Author

wongpeiyi commented Aug 22, 2019

@sukima is right, https://github.com/emberjs/ember.js/blob/master/packages/%40ember/modifier/index.ts is still exported with a typo capabilties

@pzuraq sorry to tag you here – not sure if you missed out the above typo correction in emberjs/ember.js@b8a5633 or if it was intentionally left as such.

Also if you have any alternative ideas how best to support 3.7 (with typo) until your commit (see: #13 (comment)) that would help!

@pzuraq
Copy link

pzuraq commented Aug 22, 2019

@wongpeiyi you are correct, that was missed, but it shouldn't affect public consumers. That file is not what ultimately gets exported and used currently, there is a babel transform that will convert usages of capabilities to Ember._modifierManagerCapabilities. We should definitely still fix it, the long term plan is to drop that babel transform (sooner rather than later hopefully) but otherwise everything should be working.

@wongpeiyi
Copy link
Contributor Author

@pzuraq

That file is not what ultimately gets exported

I'm aware of that, it was a side observation that just came up in the earlier comments 😃

but otherwise everything should be working.

This is the bigger issue... The import does work in canary, but not from 3.7 until before your commit unless we also have a babel transform that maps Ember._modifierManagerCapabilties (typo). I can create a separate issue on ember source if that's better?

@rwjblue
Copy link
Member

rwjblue commented Aug 23, 2019

FYI - The build failed due to a package bumping to Node 8 only deps without a major version bump, that was just fixed (SBoudrias/Inquirer.js#837) and our CI should be 👍 again. I've restarted the floating dependencies job here to confirm.

@rwjblue
Copy link
Member

rwjblue commented Aug 23, 2019

RE: the typo, we should probably backport the typo fix to 3.8 and 3.12 so it's less of an issue over time (@wongpeiyi would you mind making an issue in Ember for that?)

@rwjblue
Copy link
Member

rwjblue commented Aug 23, 2019

Working on making the extra addon/modifiers.js not needed over in ember-polyfills/ember-modifier-manager-polyfill#10, will update here once thats landed and released.

[email protected] includes support for ensuring
`import { capabilities } from '@ember/modifier'` works back to 2.12
(hiding the typo issue).
@rwjblue
Copy link
Member

rwjblue commented Aug 23, 2019

@sukima ^ that last commit (94fab06) will be useful to ember-oo-modifiers also

@rwjblue
Copy link
Member

rwjblue commented Aug 23, 2019

OK, @wongpeiyi I've pushed a commit that updates to [email protected] which ensures that import { capabilities } from '@ember/modifiers' works for the full range of supported Ember versions (for Ember 3.8 through 3.13.0-beta.2 it only fixes the typo of Ember._modifierManagerCapabilties).

@rwjblue rwjblue added the bug Something isn't working label Aug 23, 2019
@rwjblue rwjblue changed the title Add capabilities to modifier managers Avoid modifier manager capabilities deprecation Aug 23, 2019
@rwjblue rwjblue merged commit 7544e8c into emberjs:master Aug 23, 2019
@rwjblue
Copy link
Member

rwjblue commented Aug 23, 2019

Released in v1.0.1.

@wongpeiyi
Copy link
Contributor Author

Thanks @rwjblue that was quick

we should probably backport the typo fix to 3.8 and 3.12 so it's less of an issue over time (@wongpeiyi would you mind making an issue in Ember for that?)

Am I right in assuming that issue is not needed anymore?

Copy link
Member

rwjblue commented Aug 24, 2019

Am I right in assuming that issue is not needed anymore?

Yes, I just hadn't thought of fixing in the polyfill at the time (but once I did, it seemed obviously better). Thank you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants