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

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 this issue Aug 10, 2021 · 5 comments

Comments

@void-mAlex
Copy link

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
Copy link
Contributor

dcyriller commented Aug 16, 2021

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:

  • remove ember-cached-decorator-polyfill from the shared addon (provided you don’t use the cached decorator in the addon)
  • add ember-cached-decorator-polyfill in the apps’ dependencies

@void-mAlex
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

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

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

No branches or pull requests

3 participants