-
Notifications
You must be signed in to change notification settings - Fork 6
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 polyfill works properly with Ember 3.27+ #75
Conversation
c22ef6d
to
1d459ed
Compare
FYI, I have tried this in our medium-sized app with ember-source 3.27 (and TypeScript) and it worked fine! |
Thanks for working on this. My concern with this PR is that the regex-based rewriting looks pretty fragile. If Instead of replacing the existing babel plugin with a regex, I think it would be better to modify the plugin so it does the rewriting of the imported path. |
OK, I made a second try, now rewriting the import via the babel AST. I did not cover re-exporting it, but IMHO that seems OK to me. I did not 100% see through how I would make the re-export declaration work, I am not necessarily a babel-AST-pro 😅 |
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.
Thanks for picking this up! I left one minor comment (removing that new dependency that we don't need any more), but it should be good to go once that is fixed.
be8503a
to
d13c745
Compare
True, I can see the same in our app. I will check it out and see if I can make it work..! |
I can also reproduce it by updating the addon itself to [email protected] (which at least makes fixing it easier, I guess) |
OK, seems from ember-cli-babel 7.23->7.24 the |
Also update addon's ember-cli-babel version to latest
So, I have now refactored it again, it is much nicer now actually :D I am using ember-cli-babel-plugin-helpers to just add the plugin in a regular fashion, without patching anything. I have also updated ember-cli-babel in this addon itself to latest, and the tests are passing fine. Can @acorncom you try it again in the repro? |
Confirmed that it's no longer crashing in my repro and that the |
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 much better, thank you @mydea!
Unfortunately, I am still seeing the error:
I'm pretty sure I had the latest ember-cached-decorator-polyfill installed, and worse is it failed even with [email protected] |
Hmm, I just tried it in our app: [email protected] And I have tried it with: [email protected] Both of these setups worked fine for me. |
I have tried these combinations as well but still no luck, I even use |
Based on ef4's comment here: #70 (comment)
This PR changes how the polyfill works by simply rewriting
import { cached } from '@glimmer/tracking'
toimport { cached } from 'ember-cached-decorator-polyfill'
.It uses a simple regex to do so - I added some tests covering all the different import scenarios I could think of (as
@glimmer/tracking
only has a single other relevant importtracked
, it is relatively straightforward).I hope I didn't miss anything critical 😅 but the tests are passing and it seems to be working just fine.