-
-
Notifications
You must be signed in to change notification settings - Fork 120
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
Move custom options to "ember-cli-babel" options hash #105
Conversation
This will allow Ember CLI to detect what config key "compileModules" needs to be put into
|
since those are the only two options that
do we have some sort of test harness for that? |
@nathanhammond got tests now for the new logic |
var customOptions = addonOptions['ember-cli-babel']; | ||
|
||
if (customOptions && 'includePolyfill' in customOptions) { | ||
return customOptions.includePolyfill === true; |
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 we check to see if it is in both places here as well and writeDeprecateLine
?
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.
FYI - we can't rely on writeDeprecateLine as far back as this lib supports
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.
@rwjblue And I was proud of knowing this because of recent spelunking: #105 (comment)
It turns out that having an in-built ecosystem mental model is absurdly valuable...
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.
@rwjblue since the deprecation will only be triggered in recent CLIs we can just use writeDeprecateLine
here
@nathanhammond I'll move the deprecation check in front of everything
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.
Good point RE, newer CLI's only doing the deprecation. Maybe you can leave an inline comment (so future selves don't see it's use here as an excuse to use it for other ember-CLI deprecations)? I don't feel strongly either way though...
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.
done
} else { | ||
ui.writeLine(message, 'ERROR'); | ||
_getAddonOptions: function() { | ||
return (this.parent && this.parent.options) || (this.app && this.app.options) || {}; |
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.
That second check shouldn't be required unless we're going way back in support. (Before 0.2.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.
@nathanhammond that is pretty much the original code and not modified by me (see https://github.com/babel/ember-cli-babel/blob/b9a0dc232f4038bf6c95de51f71b04faff08406d/index.js#L76)
var customOptions = addonOptions['ember-cli-babel']; | ||
|
||
if (customOptions && 'compileModules' in customOptions) { | ||
return customOptions.compileModules === true; |
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.
Same story: can we check to see if it is in both places here as well and writeDeprecateLine
?
} | ||
_getBabelOptions: function() { | ||
var addonOptions = this._getAddonOptions(); | ||
var options = clone(addonOptions.babel || {}); |
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 we name options
a bit more clearly? I had to look at this multiple times to figure out what it was supposed to be.
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.
options
was the original name. I kept it to try to keep the diff smaller.
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.
👍
options.blacklist.push('es6.modules'); | ||
|
||
if (compileModules) { | ||
if (options.blacklist.indexOf('es6.modules') >= 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.
Let's save this off, you use it three times.
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.
again: this existed already before this PR (see https://github.com/babel/ember-cli-babel/blob/b9a0dc232f4038bf6c95de51f71b04faff08406d/index.js#L125-L135)
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.
Yes, but you already modify those lines, so let's address this one.
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.
done
After these changes I'm pretty sure it's good to go. Would like a bonus confirmation from @rwjblue. |
@@ -16,6 +17,7 @@ module.exports = { | |||
var dep = checker.for('ember-cli', 'npm'); | |||
|
|||
this._shouldSetupRegistryInIncluded = !dep.satisfies('>=0.2.0'); | |||
this._shouldShowBabelDeprecations = !dep.lt('2.11.0-beta.1'); |
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.
I think this should be < 2.11.0-beta.2
since 2.11.0-beta.1 didn't include the code to prevent deprecations...?
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.
@rwjblue I'm fine with either
Agreed, I am 👍 once the current round of requests are resolved. We should followup with updates in ember-cli's beta branch ASAP though... |
@rwjblue @nathanhammond addressed your requests |
Prevent running babel twice. We currently transpile the output of all addon's `addon/` trees twice. Once inside the addon's `treeForAddon` step (this one excludes module transpilation by adding `es6.modules` to babel's blacklist) and the second time in [lib/broccoli/ember-app.js](https://github.com/ember-cli/ember-cli/blob/2df251ce018fd12570bdce72c167ff14a427fa74/lib/broccoli/ember-app.js#L929). This was originally done because we didn't require that addon's include their own preprocessors for JS (mostly due to performance reasons IIRC). In the time since that decision was made, we have swapped from a few different systems `es6-module-transpiler` -> `esperanto` -> `babel`. Now that our module transpilation step is using the same thing as we recommend addon's use (`ember-cli-babel`) what we are doing seems pretty whacky. This PR removes the double processing of addon's `addon/` files through babel, and includes a deprecation message for the scenario that originally prompted the second pass (where an addon didn't include a module transpiling preprocessor). TODO: - [x] Confirm that we have tests that already confirm this is working properly (I believe that our existing nested addon tests cover these changes, but I'd like to confirm). - [ ] Consider / discuss backporting the deprecation message for addons that do not have a transpiler and are relying on the global processing step. - [X] Add helpful error if an addon overrides `this.options.babel.compileModules` - [X] Implement logic to avoid deprecation being added in emberjs/ember-cli-babel#105. - [X] Split out `findAddonByName` into a utility method to be shared - [ ] r? Addresses parts of #5015
Prevent deprecation from `ember-cli-babel` config options. Future versions of ember-cli-babel will be moving the location for its own configuration options out of `babel` and will be issuing a deprecation if used in the older way. This changes things around to detect the correct (undeprecated) options location, and use it when possible. Otherwise it falls back to `babel` (which is backwards compatible without a deprecation and forwards compatible with a deprecation). /cc @Turbo87 Reference: emberjs/ember-cli-babel#105
Resolves #102 and unblocks part of the journey to Babel 6
/cc @rwjblue @stefanpenner @nathanhammond