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 generated global MemberExpressions are not shared #116

Merged
merged 2 commits into from
Jun 2, 2020

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 1, 2020

The original fix in b63bb6e 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. 🤦

@rwjblue rwjblue force-pushed the more-failure-cases branch from d10356f to 1431b51 Compare June 2, 2020 14:30
The original fix in b63bb6e 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 marked this pull request as ready for review June 2, 2020 15:02
@rwjblue rwjblue changed the title Add a failing test for ember-google-maps/utils/helper scenario. Ensure generated global MemberExpressions are not shared Jun 2, 2020
@rwjblue rwjblue added the bug label Jun 2, 2020
@rwjblue rwjblue merged commit f69d769 into master Jun 2, 2020
@rwjblue rwjblue deleted the more-failure-cases branch June 2, 2020 15:23
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.

1 participant