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 conflicting transforms for @ember/debug. #161

Merged
merged 4 commits into from Jul 6, 2017

Conversation

Projects
None yet
4 participants
@rwjblue
Copy link
Member

rwjblue commented Jul 3, 2017

When both babel-plugin-debug-macros and babel-plugin-ember-modules-api-polyfill attempt to replace import { assert } from '@ember/assert'; the interaction between the two also removes any helpers that are injected by other parts of the transpilation (e.g. _classCallCheck or _asyncToGenerator) therefore making the transpiled output fail at runtime.

This updates babel-plugin-ember-modules-api-polyfill to a version that allows specifying a blacklist so that we can avoid this conflict.

Fixes #160

Avoid conflicting transforms for @ember/debug.
When both babel-plugin-debug-macros and babel-plugin-ember-modules-api-polyfill
attempt to replace `import { assert } from '@ember/assert';` the interaction
between the two also removes any helpers that are injected by other parts of
the transpilation (e.g. `_classCallCheck` or `_asyncToGenerator`) therefore
making the transpiled output fail at runtime.

This updates babel-plugin-ember-modules-api-polyfill to a version that allows
specifying a blacklist so that we can avoid this conflict.

@rwjblue rwjblue requested a review from Turbo87 Jul 3, 2017

index.js Outdated
@@ -260,7 +260,7 @@ module.exports = {
if (this._emberVersionRequiresModulesAPIPolyfill()) {
const ModulesAPIPolyfill = require('babel-plugin-ember-modules-api-polyfill');

return [[ModulesAPIPolyfill]];
return [[ModulesAPIPolyfill, { blacklist: ['@ember/debug'] }]];

This comment has been minimized.

@Turbo87

This comment has been minimized.

@rwjblue

rwjblue Jul 3, 2017

Author Member

I don't think all of those are required (e.g. this blacklist doesn't prevent @ember/debug/data-adapter or @ember/debug/container-debug-adapter). The only one that is not supported by babel-plugin-debug-macros is Ember.inspect (which seems misplaced in @ember/debug IMHO).

This comment has been minimized.

@Turbo87

Turbo87 Jul 3, 2017

Contributor

so import { inspect } from '@ember/debug'; won't work at all after this PR?

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Jul 4, 2017

Updated to properly allow named exports from @ember/debug to be handled by the "proper" plugin (and added a test to confirm).

@Turbo87 - Ready for another look...

@rwjblue rwjblue requested a review from Turbo87 Jul 4, 2017

@stefanpenner

This comment has been minimized.

Copy link
Member

stefanpenner commented Jul 4, 2017

oh babel...

@Turbo87

Turbo87 approved these changes Jul 4, 2017

@Turbo87

This comment has been minimized.

Copy link
Contributor

Turbo87 commented Jul 4, 2017

It looks like @ember/debug is still not transpiled properly with the update:

define('ct/controllers/search', ['exports', 'ember', 'ct/utils/pnr_utils', 'ct/computed',
  'ct/utils/transition', 'ct/utils/book_context', 'ct/constants', 'ct/mixins/errors_support',
  '@ember/debug'], function (exports, _ember, _pnr_utils, _computed, _transition,
  _book_context, _constants, _errors_support) {

From what I understand I shouldn't see an @ember/debug import here at all, but even if I should then there should be a corresponding parameter in the function definition, right?

@Turbo87

This comment has been minimized.

Copy link
Contributor

Turbo87 commented Jul 4, 2017

I think we might have to change the order of:

       this._getDebugMacroPlugins(config),
       this._getEmberModulesAPIPolyfill(config),

Update: I just tried that with a clean cache, but it didn't seem to make a difference

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Jul 4, 2017

Ordering is totally screwed up. Changing the order actually has no effect due to the way babel merges all plugins into a single AST visitor.

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Jul 4, 2017

@Turbo87 - Can you share the raw source of the imports for that snippet? Can you also confirm if the thing imported from @ember/debug is actually being used?

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Jul 6, 2017

@Turbo87 - OK, fixed that last issue I believe. Can you give it a try again?

For more info on what was wrong, you can see the writeup in ember-cli/babel-plugin-debug-macros#45.

@rwjblue rwjblue requested a review from Turbo87 Jul 6, 2017

@Turbo87

This comment has been minimized.

Copy link
Contributor

Turbo87 commented Jul 6, 2017

@rwjblue sorry to disappoint you... 😞

it now correctly transpiled the debug macros things, but it no longer converts the module imports at all 🤔

define('ct/app', ['exports', '@ember/application', 'ember', 'ct/resolver',
  'ember-load-initializers', 'ct/environment'], function (exports, _application,
  _ember, _resolver, _emberLoadInitializers, _environment) {

@rwjblue rwjblue force-pushed the rwjblue:avoid-issues-with-ember-debug branch from 40971cb to 4f9a45e Jul 6, 2017

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Jul 6, 2017

@Turbo87 - I added a few more tests/assertions around non-@ember/debug usage just to add a bit more coverage. I'm trying to reproduce, but can't seem to...

@Turbo87

This comment has been minimized.

Copy link
Contributor

Turbo87 commented Jul 6, 2017

looks like clearing my caches resolved the issue. this seems good to go 👍

@Turbo87

Turbo87 approved these changes Jul 6, 2017

@rwjblue rwjblue merged commit 6ac2390 into babel:master Jul 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rwjblue rwjblue deleted the rwjblue:avoid-issues-with-ember-debug branch Jul 6, 2017

@Turbo87 Turbo87 added the bug label Aug 31, 2017

@Dhaulagiri

This comment has been minimized.

Copy link

Dhaulagiri commented Aug 31, 2017

(pardon my rudimentary understanding) but with this change if I try an import like import { warn } from '@ember/debug'; in an addon I'm working with running ember-cli-babel 6.8.2 , warn is undefined because its transpilation back to an Ember global is blacklisted.

If it's helpful I can open a new issue and try to make a reproduction, but perhaps there is something I'm not understanding about the intent of this PR?

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Sep 13, 2017

@Dhaulagiri - We have two plugins that both know how to operate on import { warn } from '@ember/debug';. The first is babel-plugin-ember-modules-api-polyfill which will remove the import and replace (essentially) with Ember.* usage. The second is babel-plugin-debug-macros which will replace these specific debug statements with customized debug build only output. This PR is all about preventing these two plugins from conflicting by disabling the modules-api-polyfill (because we want that prod build stripping yumminess).

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Sep 13, 2017

Checkout this test for example output (and confirmation that we are indeed processing these).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment