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

Update babel-plugin-ember-modules-api-polyfill to fix issues with invalid identifiers. #338

Merged
merged 2 commits into from
May 29, 2020

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented May 29, 2020

The babel plugin 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 has caused us quite a bit of pain 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), and those plugins properly sanitizing invalid identifiers.

The plugin was updated to avoid this mistake, and properly rewrite the identifiers with a member expression. It also added tests to confirm things are working as intended.

Fixes #316
Fixes #322
Fixes emberjs/ember.js#18992

…alid identifiers.

The babel plugin 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 has caused us quite a bit of pain 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), and
those plugins **properly** sanitizing invalid identifiers.

The plugin was updated to avoid this mistake, and properly rewrite the
identifiers with a member expression. It also added tests to confirm
things are working as intended.
@rwjblue rwjblue added the bug label May 29, 2020
This previously passed by _relying_ on the bug that was fixed in the
modules API polyfill.
@rwjblue rwjblue merged commit 73ac6bf into master May 29, 2020
@rwjblue rwjblue deleted the fix-invalid-identifier-issues branch May 29, 2020 02:28
@lukemelia
Copy link
Member

Thanks for working on this @rwjblue!

I tried upgraded an addon that was having an issue with this (ember-keyboard), and while my yarn.lock-driven tests pass, my 3.12 ember-try build now fails with a different error:

➜  ember-keyboard git:(rfc/api-change) ✗ node_modules/.bin/ember try:one ember-lts-3.12
yarn install v1.22.4
[snip]
Build Error (broccoli-persistent-filter:Babel > [Babel: @glimmer/component]) in @glimmer/component/-private/ember-component-manager.ts

/Users/lmelia/p/ember/ember-keyboard/@glimmer/component/-private/ember-component-manager.ts: Property id of TSDeclareFunction expected node to be of a type ["Identifier"] but instead got "MemberExpression"

Let me know if it would be best for me to open a separate issue somewhere, or if I can provide any additional info.

@miguelcobain
Copy link

The same issue started happening to me as well on two of my apps.

@BarryThePenguin
Copy link

BarryThePenguin commented May 29, 2020

I'm seeing the same error in a number of apps and addons with ember-cli-babel@7.20.1

Apps are on v3.12
Addons, some v3.12, with a couple on v3.16

It looks like a mixed bag, so I'm taking a look to see why some would be failing while others aren't..

Build Error (broccoli-persistent-filter:Babel > [Babel: @ember-data/store]) in -private/system/model/internal-model.ts
--
  |  
  | /frontend/packages/activity-tracker/-private/system/model/internal-model.ts: Property left of TSQualifiedName expected node to be of a type ["TSEntityName"] but instead got "MemberExpression"

While another reads

Build Error (broccoli-persistent-filter:Babel > [Babel: @glimmer/component]) in @glimmer/component/-private/ember-component-manager.ts
--
  |  
  | /frontend/packages/get-config/@glimmer/component/-private/ember-component-manager.ts: Property id of TSDeclareFunction expected node to be of a type ["Identifier"] but instead got "MemberExpression"

@rwjblue
Copy link
Member Author

rwjblue commented May 29, 2020

https://github.com/babel/ember-cli-babel/releases/tag/v7.20.2 is out with the fix for the TS issue (thank you @fivetanley!). See ember-cli/babel-plugin-ember-modules-api-polyfill#110 for more explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants