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

Ensure Ember modules API related globals do not share AST Nodes #348

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 2, 2020

The original fix in ember-cli/babel-plugin-ember-modules-api-polyfill#109 attempted to cache the generated member expressions, so that we didn't do duplicated work for each reference to a given global. Unfortunately, this optimization failed to take into consideration that most babel plugins work by directly mutating the node in question. In practice what that meant was that once any usage of a given global was needed to be transformed (e.g. say you had computed(...args, function(){}) and are transpiling for IE11), then all usages of that global would be mutated.

A more concrete example.

// input
import { computed } from '@ember/object';

function specialComputed(dependentKeys) {
  return computed(...dependentKeys, function() {});
}

function otherLessSpecialComputed() {
  return computed('stuff', 'hard', 'coded', function() {});
}

In this example, the first method (specialComputed) needs to be transpiled to something akin to (most of the changes here are from @babel/plugin-transform-spread):

function specialComputed(dependentKeys) {
  return Ember.computed.apply(Ember, dependentKeys.concat([function() {}]));
}

Unfortunately, since the generated MemberExpression for Ember.computed is shared, this forced the other computed usage to be transpiled to:

function otherLessSpecialComputed() {
  return Ember.computed.apply('stuff', 'hard', 'coded', function() {});
}

Which is clearly, totally invalid. 🤦

Fixes #346

The original fix in
ember-cli/babel-plugin-ember-modules-api-polyfill#109 attempted to cache
the generated member expressions, so that we didn't do duplicated work
for each reference to a given global. Unfortunately, this optimization
failed to take into consideration that most babel plugins work by
directly mutating the node in question. In practice what that meant was
that once _any_ usage of a given global was needed to be transformed
(e.g. say you had `computed(...args, function(){})` and are transpiling
for IE11), then all usages of that global would be mutated.

A more concrete example.

```js
// input
import { computed } from '@ember/object';

function specialComputed(dependentKeys) {
  return computed(...dependentKeys, function() {});
}

function otherLessSpecialComputed() {
  return computed('stuff', 'hard', 'coded', function() {});
}
```

In this example, the first method (`specialComputed`) needs to be
transpiled to something akin to (most of the changes here are from
`@babel/plugin-transform-spread`):

```js
function specialComputed(dependentKeys) {
  return Ember.computed.apply(Ember, dependentKeys.concat([function() {}]));
}
```

Unfortunately, since the generated `MemberExpression` for
`Ember.computed` is shared, this forced the other `computed` usage to be
transpiled to:

```js
function otherLessSpecialComputed() {
  return Ember.computed.apply('stuff', 'hard', 'coded', function() {});
}
```

Which is clearly, **totally** invalid. 🤦
@rwjblue rwjblue added the bug label Jun 2, 2020
@rwjblue rwjblue merged commit f1a6cd6 into master Jun 2, 2020
@rwjblue rwjblue deleted the update-modules-api-polyfill branch June 2, 2020 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Another _Ember is undefined error
1 participant