ember-polyfills / ember-cached-decorator-polyfill

Polyfill for RFC 566: @cached
MIT License
20 stars 6 forks source link

Ensure polyfill works properly with Ember 3.27+ #75

Closed mydea closed 3 years ago

mydea commented 3 years ago

Based on ef4's comment here: https://github.com/ember-polyfills/ember-cached-decorator-polyfill/issues/70#issuecomment-837176484

This PR changes how the polyfill works by simply rewriting import { cached } from '@glimmer/tracking' to import { 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 import tracked, it is relatively straightforward).

I hope I didn't miss anything critical :sweat_smile: but the tests are passing and it seems to be working just fine.

mydea commented 3 years ago

FYI, I have tried this in our medium-sized app with ember-source 3.27 (and TypeScript) and it worked fine!

ef4 commented 3 years ago

Thanks for working on this.

My concern with this PR is that the regex-based rewriting looks pretty fragile. If @glimmer/tracking adds any new exported names, it's likely to break.

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.

mydea commented 3 years ago

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 :sweat_smile:

acorncom commented 3 years ago

@mydea just pushed up a simple repro I'd built to test things as part of https://github.com/ember-polyfills/ember-cached-decorator-polyfill/issues/77 Ran it against your branch here and it seems to still be failing

mydea commented 3 years ago

True, I can see the same in our app. I will check it out and see if I can make it work..!

mydea commented 3 years ago

I can also reproduce it by updating the addon itself to ember-cli-babel@7.26.6 (which at least makes fixing it easier, I guess)

mydea commented 3 years ago

OK, seems from ember-cli-babel 7.23->7.24 the _getEmberModulesAPIPolyfill hook is dropped, leading to the whole babel plugin not running.

mydea commented 3 years ago

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?

acorncom commented 3 years ago

Confirmed that it's no longer crashing in my repro and that the @cache decorator seems to be working smoothly

nightire commented 3 years ago

Unfortunately, I am still seeing the error:

Uncaught TypeError: decorator is not a function

I'm pretty sure I had the latest ember-cached-decorator-polyfill installed, and worse is it failed even with ember-source@3.26.1

image

mydea commented 3 years ago

Unfortunately, I am still seeing the error:

Uncaught TypeError: decorator is not a function

I'm pretty sure I had the latest ember-cached-decorator-polyfill installed, and worse is it failed even with ember-source@3.26.1

image

Hmm, I just tried it in our app:

ember-cached-decorator-polyfill@0.1.2 ember-source@3.26.1 ember-cli-babel@7.26.5

And I have tried it with:

ember-cached-decorator-polyfill@0.1.2 ember-source@3.27.1 ember-cli-babel@7.26.6

Both of these setups worked fine for me.

nightire commented 3 years ago

Hmm, I just tried it in our app:

ember-cached-decorator-polyfill@0.1.2 ember-source@3.26.1 ember-cli-babel@7.26.5

And I have tried it with:

ember-cached-decorator-polyfill@0.1.2 ember-source@3.27.1 ember-cli-babel@7.26.6

Both of these setups worked fine for me.

I have tried these combinations as well but still no luck, I even use resolutions to constraint these versions, and the error still remains. I'm out of options now.😕