ember-polyfills / ember-cached-decorator-polyfill

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

addon no longer works when declared as a "dependency" from an addon with v0.1.4 and DOES work with v0.1.3 #97

Open void-mAlex opened 3 years ago

void-mAlex commented 3 years ago

in _applyDecoratedDescriptor code of the component in built version the decorator is undefined for 0.1.4 and it works for 0.1.3

setup is ember-cached-decorator-polyfill declared as a 'dependency' in a shared addon decorator used in app that includes the shared addon.

I can create a reproduction if required, but I can confirm it used to work on version 0.1.3

dcyriller commented 3 years ago

I think the behavior you’re describing would be part of the bug fixed in the cross linked PR.

To me (note: this could be checked with a maintainer), ember-cached-decorator-polyfill should be present in the project that makes use of the cached decorator. If you write an addon and use the cached decorator in the addon’s code, the polyfill should be in the addon’s dependencies. If you write an app and use the cached decorator in the app’s code, the polyfill should be in the app’s dependencies. This allows us to run the Babel plugin only on the code that makes use of the cached decorator. Unfortunately there was a bug in 0.1.3. When the ember-cached-decorator-polyfill was in the dependencies of an addon, the Babel plugin was not registered to transpile the addon’s code but the host app’s instead.

So, to upgrade to 0.1.4 you would have to:

void-mAlex commented 3 years ago

@dcyriller I'm trying really hard to not manage the version of cache decorators in all 7+ other apps and addons; that the "bug" of <0.1.4 has enabled us to use it the decorator for. It's not a maintainable solution to add the decorator to every one of our apps, so with that in mind I will attempt to set it as a peerDependencies and see how far I get.

dcyriller commented 3 years ago

I hear it is painful for you (and other users) to add the dependency in every one of your apps. When you want to upgrade the polyfill, you have to bump it in 7+ apps.

A possible solution would be to register the babel plugin for both the addon tree and the host app tree. On the plus side, it would solve this issue. On the minus side, it sounds vaguely odd from a performance perspective. Indeed, every app depending on an addon depending on the polyfill would get transpiled with the babel plugin. As more and more addons are starting to use this polyfill (ember-data will start using it in 3.18), it means that more and more apps would get transpiled with the babel plugin.

simonihmig commented 2 years ago

It's not a maintainable solution to add the decorator to every one of our apps

Sorry, don't want to sound ignorant, but I genuinely don't understand what the problem is? Adding one line to package.json?

The semantics of dependencies, especially for build-time transforms like polyfills, is pretty consistent: when you need it, you need to declare it a dependency. When you need Babel transpilation in your app (all do), the app needs ember-cli-babel. Doesn't help if an addon depends on it, that's only for the addon's concerns. Same for ember-cli-htmlbars, all those polyfills etc. Even for plain imports when using ember-auto-import or Embroider: you can only import what you depend on.

void-mAlex commented 2 years ago

Sorry, don't want to sound ignorant, but I genuinely don't understand what the problem is? Adding one line to package.json?

it's desired that between the 7 (and growing number) of apps that make use of the decorator they all share the same version. in that scenario adding 1 line to the package json of each app, while doable is long term maintenance heavy. this gets compounded by a number of other packages that would require the same treatment if delivering them through an addon was not possible which brings me to the second point you made

The semantics of dependencies, especially for build-time transforms like polyfills, is pretty consistent: when you need it, you need to declare it a dependency. When you need Babel transpilation in your app (all do), the app needs ember-cli-babel. Doesn't help if an addon depends on it, that's only for the addon's concerns. Same for ember-cli-htmlbars, all those polyfills etc. Even for plain imports when using ember-auto-import or Embroider: you can only import what you depend on.

if I read this correctly importing something from the app that is not declared in the dependencies of the app should not work. while putting Embroider aside for a second, I can 100% say that it does work for all of the packages we're using from lodash-es to other ember addons.

what that allows us to do is have only 1 internal addon that manages the versions of a dozen or so packages and the other apps only depend on that internal addon for updates. this also ensures that we avoid packages overriding themselves with different versions in the monorepo.

for clarity the lodash-es package mentioned above is declared only in the addon as a dependency but used from the apps