-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
Blacklists @ember/string
if dependency is present
#176
Conversation
locks
commented
Aug 11, 2017
•
edited
Loading
edited
- Add behaviour
- Test behaviour
@locks what the test? use case I want help you on this |
@locks - Looks good, I just released the polyfill with the required changes, can you rebase and update the minimum version? |
@villander we need to check that we are actually detecting |
@ember/string
if dependency is present
index.js
Outdated
@@ -253,14 +253,19 @@ module.exports = { | |||
}, | |||
|
|||
_getEmberModulesAPIPolyfill(config) { | |||
const blacklist = { '@ember/debug': ['assert', 'deprecate', 'warn'] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you extract the logic here into a _getEmberModulesAPIBlacklist()
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed, was that what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, thank you
We are G.T.G.™ |
index.js
Outdated
_emberStringDependencyPresent() { | ||
let checker = new VersionChecker(this.parent).for('@ember/string', 'npm'); | ||
|
||
return checker.isAbove('0.0.0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why .isAbove()
? I think there is an .exists()
now 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, confirm. But only recently IIRC.
If the `@ember/string` package is present in the dependencies it means that we should not convert the imports for those modules into globals, because the modules will be provided by the package itself.