Ensure identifiers are properly re-written. #109

rwjblue · 2020-05-29T02:00:53Z

The code previously took an identifier and replaced it's name value with one that included a string like Ember.Foo. This is completely invalid (a string like Ember.Foo is actually a MemberExpression), but we got away with it because for the most part Babel happily printed whatever value you give it in that spot.

Unfortunately, this happy mistake bit us fairly severely due to Babel introducing new plugins that run as part of @babel/preset-env (e.g. to make sure that all identifiers would work properly on IE9).

This commit changes from doing a path.scope.rename(importName, globalPathString) to properly replacing the identifier with a member expression.

This PR includes the failing test added in #108 (see that PR to confirm the failure mode), and ensures that it passes.

Fixes #64
Closes #108

This reproduces the errors being reported that are ultimately caused by this libraries mutation of `Identifier` nodes to include things like `Ember.Application` (which is fundamentally not a valid identifier value, it is a `MemberExpression`).

The code previously took an identifier and replaced it's `name` value with one that included a string like `Ember.Foo`. This is completely invalid (a string like `Ember.Foo` is actually a MemberExpression), but we got away with it because for the most part Babel happily printed **whatever** value you give it in that spot. Unfortunately, this happy mistake bit us fairly severely due to Babel introducing new plugins that run as part of `@babel/preset-env` (e.g. to make sure that all identifiers would work properly on IE9). This commit changes from doing a `path.scope.rename(importName, globalPathString)` to properly replacing the identifier with a member expression.

[ember-cli#109](5de3c79) introduced a safer way to rewrite variable names by generating MemberExpressions to fix code for IE9, but this introduced a regression for packages using `@babel/plugin-transform-typescript`, such as Ember Data. After 109, Babel would throw the following error if `babel-plugin-ember-modules-api-polyfill` attempetd to rewrite a type information node (in this case, `TSQualifiedName`), if the file also used the import in runtime JS code. The code that triggers this looks like: ```javascript import { default as RSVP, Promise } from 'rsvp'; // value must be used to trigger this RSVP.Promise.resolve().then(() => {}); // AND the same import local name must be used as a type as well function scheduleSave(identifier: RecordIdentifier, options: any = {}): RSVP.Promise<null | SingleResourceDocument> { } ``` ``` Property left of TSQualifiedName expected node to be of a type ["TSEntityName"] but instead got "MemberExpression" ``` This was being triggered when `ember-data` was compiled. Examples were taken from Ember Data's codebase: - https://github.com/emberjs/data/blob/70b0c55e1a950bed1da64d0ecb4eaa0d5df92f9f/packages/store/addon/-private/system/fetch-manager.ts#L124 - https://github.com/emberjs/data/blob/70b0c55e1a950bed1da64d0ecb4eaa0d5df92f9f/packages/store/addon/-private/system/fetch-manager.ts#L124 - https://github.com/emberjs/data/blob/70b0c55e1a950bed1da64d0ecb4eaa0d5df92f9f/packages/store/addon/-private/system/fetch-manager.ts#L77 This change uses the [isTSQualifiedName helper](https://babeljs.io/docs/en/babel-types#tsqualifiedname) from `@babel/types` if it is available (if on a recent version of babel or if `@babel/plugin-transform-typescript` is loaded as a plugin) to determine whether or not to rewrite the node. Since type information is removed from Babel output, and babel does not do any linting/type checking of transformed Typescript, it seems fine to leave these nodes alone.

[ember-cli#109](5de3c79) introduced a safer way to rewrite variable names by generating MemberExpressions to fix code for IE9, but this introduced a regression for packages using `@babel/plugin-transform-typescript`, such as Ember Data. After 109, Babel would throw the following error if `babel-plugin-ember-modules-api-polyfill` attempted to rewrite a type information node (in this case, `TSQualifiedName`), if the file also used the import in runtime JS code. The code that triggers this looks like: ```javascript import { default as RSVP, Promise } from 'rsvp'; // value must be used to trigger this RSVP.Promise.resolve().then(() => {}); // AND the same import local name must be used as a type as well function scheduleSave(identifier: RecordIdentifier, options: any = {}): RSVP.Promise<null | SingleResourceDocument> { } ``` ``` Property left of TSQualifiedName expected node to be of a type ["TSEntityName"] but instead got "MemberExpression" ``` This was being triggered when `ember-data` was compiled. Examples were taken from Ember Data's codebase: - https://github.com/emberjs/data/blob/70b0c55e1a950bed1da64d0ecb4eaa0d5df92f9f/packages/store/addon/-private/system/fetch-manager.ts#L124 - https://github.com/emberjs/data/blob/70b0c55e1a950bed1da64d0ecb4eaa0d5df92f9f/packages/store/addon/-private/system/fetch-manager.ts#L124 - https://github.com/emberjs/data/blob/70b0c55e1a950bed1da64d0ecb4eaa0d5df92f9f/packages/store/addon/-private/system/fetch-manager.ts#L77 This change uses the [isTSQualifiedName helper](https://babeljs.io/docs/en/babel-types#tsqualifiedname) from `@babel/types` if it is available (if on a recent version of babel or if `@babel/plugin-transform-typescript` is loaded as a plugin) to determine whether or not to rewrite the node. Since type information is removed from Babel output, and babel does not do any linting/type checking of transformed Typescript, it seems fine to leave these nodes alone.

[ember-cli#109](5de3c79) introduced a safer way to rewrite variable names by generating MemberExpressions to fix code for IE9, but this introduced a regression for packages using `@babel/plugin-transform-typescript`, such as Ember Data. After 109, Babel would throw the following error if `babel-plugin-ember-modules-api-polyfill` attempted to rewrite a type information node (in this case, `TSQualifiedName`), if the file also used the import in runtime JS code. The code that triggers this looks like: ```javascript import { default as RSVP, Promise } from 'rsvp'; // value must be used to trigger this RSVP.Promise.resolve().then(() => {}); // AND the same import local name must be used as a type as well function scheduleSave(identifier: RecordIdentifier, options: any = {}): RSVP.Promise<null | SingleResourceDocument> { } ``` ``` Property left of TSQualifiedName expected node to be of a type ["TSEntityName"] but instead got "MemberExpression" ``` This was being triggered when `ember-data` was compiled. Examples were taken from Ember Data's codebase: - https://github.com/emberjs/data/blob/70b0c55e1a950bed1da64d0ecb4eaa0d5df92f9f/packages/store/addon/-private/system/fetch-manager.ts#L124 - https://github.com/emberjs/data/blob/70b0c55e1a950bed1da64d0ecb4eaa0d5df92f9f/packages/store/addon/-private/system/fetch-manager.ts#L124 - https://github.com/emberjs/data/blob/70b0c55e1a950bed1da64d0ecb4eaa0d5df92f9f/packages/store/addon/-private/system/fetch-manager.ts#L77 This change looks at the `type` of the `referencePath.parentNode` to see if it belongs with `TS`, since it seems like all Typescript types in @babel/types are namespaced with `TS`. There's unfortunately no list or API of typescript nodes we could programatically pull this from. Since type information is removed from Babel output, and babel does not do any linting/type checking of transformed Typescript, it seems fine to leave these nodes alone.

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 added 2 commits May 28, 2020 21:55

rwjblue added the bug label May 29, 2020

rwjblue merged commit 54b97a3 into master May 29, 2020

rwjblue deleted the fix-identifier-rewriting branch May 29, 2020 02:03

marcoow mentioned this pull request May 29, 2020

Fix CI mainmatter/ember-simple-auth#2207

Merged

This was referenced Jun 2, 2020

Ensure Ember modules API related globals do not share AST Nodes emberjs/ember-cli-babel#348

Merged

Another _Ember is undefined error emberjs/ember-cli-babel#346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure identifiers are properly re-written. #109

Ensure identifiers are properly re-written. #109

rwjblue commented May 29, 2020 •

edited

Loading

Ensure identifiers are properly re-written. #109

Ensure identifiers are properly re-written. #109

Conversation

rwjblue commented May 29, 2020 • edited Loading

rwjblue commented May 29, 2020 •

edited

Loading