-
-
Notifications
You must be signed in to change notification settings - Fork 21
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 errors when replacing Ember modules imports that are used as both a type and a value #110
Conversation
385101d
to
fc2b219
Compare
c25a619
to
39515f6
Compare
src/index.js
Outdated
@@ -167,7 +172,9 @@ module.exports = function (babel) { | |||
let binding = path.scope.getBinding(local.name); | |||
|
|||
binding.referencePaths.forEach((referencePath) => { | |||
referencePath.replaceWith(getMemberExpressionFor(global)); | |||
if (!isTSQualifiedName(referencePath.parentPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely the right direction, but I do not believe this is the full extent of the fix. While this fixes the error in ember-data, I’m seeing errors in other projects that use TypeScript for types other than TSQualifiedName
.
I think we need to detect any type specific node and skip, not just TSQualifiedName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another common one I’ve seen is TSDeclareFunction
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had wondered about this as well! I updated the PR to check if the type
starts with TS
since it looks like all typescript types are prefixed with TS
. This doesn't feel great but I don't see a way to get a list of all the typescript types dynamically, and doing something like the starts with TS
check should be a guard against any AST types that babel/typescript adds in the future. What do you think about that approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, seems good to me, though I agree that it does seem a bit unfortunate that there isn't something better.
[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.
39515f6
to
9b38849
Compare
@fivetanley - I just pushed two small tweaks:
|
Ah, thanks. I thought I had done that, but it's early, time for coffeeeeeeee. Thanks for fixing + merging! |
https://github.com/babel/ember-cli-babel/releases/tag/v7.20.2 is out with this fix as a minimum dep version. |
#109 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 ifbabel-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:
Error looked like:
This was being triggered when
ember-data
was compiled. Examples were taken from Ember Data's codebase:This change looks at the
type
of the referencePath'sparentPath
to determine whether or not to rewrite the node. Babel does not seem to have a way to get a list of all typescript types, but does seem to prepend the type names withTS
. 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.