Avoid conflicting transforms for @ember/debug. #161

rwjblue · 2017-07-03T18:53:21Z

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

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.

Turbo87 · 2017-07-03T18:55:21Z

index.js

@@ -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'] }]];


are https://github.com/ember-cli/ember-rfc176-data/blob/master/globals.json#L74-L80 all supported in the other Babel plugin?

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).

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

rwjblue · 2017-07-04T04:20:30Z

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...

stefanpenner · 2017-07-04T05:23:21Z

oh babel...

Turbo87 · 2017-07-04T07:07:18Z

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 · 2017-07-04T08:31:42Z

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 · 2017-07-04T13:32:10Z

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 · 2017-07-04T13:34:23Z

@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 · 2017-07-06T02:58:50Z

@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.

Turbo87 · 2017-07-06T08:34:44Z

@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 · 2017-07-06T12:36:31Z

@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 · 2017-07-06T13:20:36Z

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

Dhaulagiri · 2017-08-31T20:36:13Z

(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 · 2017-09-13T23:19:31Z

@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 · 2017-09-13T23:20:26Z

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

rwjblue requested a review from Turbo87 July 3, 2017 18:53

Turbo87 reviewed Jul 3, 2017

View reviewed changes

Ensure both inspect and assert are properly supported.

4b644fe

rwjblue requested a review from Turbo87 July 4, 2017 04:20

Turbo87 approved these changes Jul 4, 2017

View reviewed changes

rwjblue mentioned this pull request Jul 5, 2017

Ensure import is removed when other tooling uses the same module. ember-cli/babel-plugin-debug-macros#45

Merged

rwjblue requested a review from Turbo87 July 6, 2017 02:59

Ensure import is removed properly when using mixed configuration.

4f9a45e

rwjblue force-pushed the avoid-issues-with-ember-debug branch from 40971cb to 4f9a45e Compare July 6, 2017 12:25

Add more tests to ensure public modules API's are stripped.

d632119

Turbo87 approved these changes Jul 6, 2017

View reviewed changes

rwjblue merged commit 6ac2390 into emberjs:master Jul 6, 2017

rwjblue deleted the avoid-issues-with-ember-debug branch July 6, 2017 13:21

Turbo87 added the bug label Aug 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid conflicting transforms for @ember/debug. #161

Avoid conflicting transforms for @ember/debug. #161

rwjblue commented Jul 3, 2017

Turbo87 Jul 3, 2017

rwjblue Jul 3, 2017

Turbo87 Jul 3, 2017

rwjblue commented Jul 4, 2017

stefanpenner commented Jul 4, 2017

Turbo87 commented Jul 4, 2017 •

edited

Turbo87 commented Jul 4, 2017 •

edited

rwjblue commented Jul 4, 2017

rwjblue commented Jul 4, 2017

rwjblue commented Jul 6, 2017

Turbo87 commented Jul 6, 2017

rwjblue commented Jul 6, 2017

Turbo87 commented Jul 6, 2017

Dhaulagiri commented Aug 31, 2017 •

edited

rwjblue commented Sep 13, 2017

rwjblue commented Sep 13, 2017

Avoid conflicting transforms for @ember/debug. #161

Avoid conflicting transforms for @ember/debug. #161

Conversation

rwjblue commented Jul 3, 2017

Turbo87 Jul 3, 2017

Choose a reason for hiding this comment

rwjblue Jul 3, 2017

Choose a reason for hiding this comment

Turbo87 Jul 3, 2017

Choose a reason for hiding this comment

rwjblue commented Jul 4, 2017

stefanpenner commented Jul 4, 2017

Turbo87 commented Jul 4, 2017 • edited

Turbo87 commented Jul 4, 2017 • edited

rwjblue commented Jul 4, 2017

rwjblue commented Jul 4, 2017

rwjblue commented Jul 6, 2017

Turbo87 commented Jul 6, 2017

rwjblue commented Jul 6, 2017

Turbo87 commented Jul 6, 2017

Dhaulagiri commented Aug 31, 2017 • edited

rwjblue commented Sep 13, 2017

rwjblue commented Sep 13, 2017

Turbo87 commented Jul 4, 2017 •

edited

Turbo87 commented Jul 4, 2017 •

edited

Dhaulagiri commented Aug 31, 2017 •

edited