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

{ type: "module" } breaks under embroider #403

Closed
ef4 opened this issue Feb 22, 2022 · 5 comments · Fixed by #404
Closed

{ type: "module" } breaks under embroider #403

ef4 opened this issue Feb 22, 2022 · 5 comments · Fixed by #404

Comments

@ef4
Copy link

ef4 commented Feb 22, 2022

ember-resource 4.3.1 has "type": "module" in package.json. This means that imports without file extensions are only legal if the package you're importing from declares exports.

But @glimmer/tracking does not declare exports, so imports like import { getValue } from '@glimmer/tracking/primitives/cache'; are not legal.

In general, you can't unilaterally adopt "type": "module" without waiting for your dependencies to be compatible with it.

This probably only breaks under embroider, when webpack is doing the full module analysis. Under ember-auto-import there is still less strictness at the boundary between a v2 addon like this one as a v1 addon like @glimmer/tracking. This repo's embroider test coverage is turned off:

# - embroider-safe
# - embroider-optimized

@NullVoxPopuli
Copy link
Owner

NullVoxPopuli commented Feb 22, 2022

Thanks for the issue report! I had thought I had taken care of this already.

however: https://github.com/NullVoxPopuli/ember-resources/runs/5290329922?check_suite_focus=true#step:5:309

It seems that @ember/test-waiters can't be found (only under embroider).

  • it is in peerDependencies in the addon
  • is a devDep in the test-app

@NullVoxPopuli
Copy link
Owner

Everything works when @ember/test-waiters is a dependency of the addon, so I just did that, and set up renovate for maximum compatibility.

@ef4
Copy link
Author

ef4 commented Feb 23, 2022

Everything works when @ember/test-waiters is a dependency of the addon, so I just did that, and set up renovate for maximum compatibility.

But... just... sigh

Can we please debug things like this instead of racing ahead with the wrong thing because it happens to work in the one test app here?

@ember/test-waiters has critical shared state in module scope. An addon bringing its own copy is very likely to lead to confusing failures in embroider apps.

Using renovate doesn't address this problem because you can't control how quickly or slowly apps update their own dependency on @ember/test-waiters.

@NullVoxPopuli
Copy link
Owner

NullVoxPopuli commented Feb 23, 2022

Can we please debug things like this instead of racing ahead with the wrong thing

yeah, but I did not believe this to be the wrong thing -- just a different thing -- based on my understanding of the below:

An addon bringing its own copy is very likely to lead to confusing failures in embroider apps.

How would this happen? I verified the dist output, and test-waiters is not pulled in to the compiled output

Using renovate doesn't address this problem because you can't control how quickly or slowly apps update their own dependency on @ember/test-waiters.

right, the setting I used is to start out with all of ^3.0.0, and told renovate to widen ranges when updates happen, which, according to the docs only would matter on a v4 release and would make the range ^3.0.0 || ^4.0.0, which, seems ok?

@ef4
Copy link
Author

ef4 commented Feb 23, 2022

How would this happen? I verified the dist output, and test-waiters is not pulled in to the compiled output

It has nothing to do with whether it will get inlined into your package. When you put something in dependencies you're telling NPM you want a copy of that package and you don't care if it's shared with anybody else.

But in the case of test-waiters, that would defeat the point of using test-waiters. If the app uses test-waiters and calls settled, you want that to wait for your addon's promises. It would not be OK for the copy of test-waiters that the app is using and the copy test-waiters that you are using to be different, because they're supposed to be inter-operating.

The only way to guarantee that you're getting the same copy is a peer dependency. You might get the same copy without a peer dependency, but that optimization can be defeated by a bunch of things outside your control. Like, some other addon could change its dependencies and suddenly cause ember-resources's promises to stop being seen by the app's settled.

NullVoxPopuli added a commit that referenced this issue Feb 23, 2022
See explanation: #403 (comment)
tldr: peerDeps are the only way to guarantee that modern packagers
only load one version (specified by the consumer)
github-actions bot pushed a commit that referenced this issue Feb 24, 2022
## [4.3.4](v4.3.3...v4.3.4) (2022-02-24)

### Bug Fixes

* `@ember/test-waiters` must be a peer dependency ([01866c7](01866c7)), closes [/github.com//issues/403#issuecomment-1048404119](https://github.com//github.com/NullVoxPopuli/ember-resources/issues/403/issues/issuecomment-1048404119)
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

Successfully merging a pull request may close this issue.

2 participants