`Module not found: Error: Can't resolve` errors with @ember-data imports in a v2 addon

jeffsawatzky commented 1 year ago

I am trying to make a v2 addon with some common ember data things that I want to share amongst some apps.

I have an example git repo here of the issue (ignore the name of the repo, I originally had different errors that started to appear once I added glint).

It has 3 commits:

setup the pnpm workspace, and include an init.sh script which shows the commands I used to generate the code.
run the init.sh script to generate the code (no other customizations at this point though)
customize the generated code (extend the ember data store, define the rollup config, etc)

If you clone the repo, then do:

pnpm install
cd packages/my-addon/addon
pnpm build
cd ../test-app
ember test

you will see errors (I hope) like:

ERROR in ../../../../addon/dist/adapters/common.js 1:0-58
Module not found: Error: Can't resolve './json-api' in 'packages/my-addon/test-app/node_modules/.embroider/rewritten-packages/@ember-data/adapter.2dada623'
 @ ../../../../addon/dist/_app_/adapters/common.js 1:0-51 1:0-51
 @ ./assets/test-app-for-my-addon.js 15:13-64

ERROR in ../../../../addon/dist/models/common/user.js 1:0-38
Module not found: Error: Can't resolve '.' in 'packages/my-addon/test-app/node_modules/.embroider/rewritten-packages/@ember-data/model.227c36cb'
 @ ../../../../addon/dist/_app_/models/common/user.js 1:0-54 1:0-54
 @ ./assets/test-app-for-my-addon.js 18:13-67

ERROR in ../../../../addon/dist/services/store.js 1:0-38
Module not found: Error: Can't resolve '.' in 'packages/my-addon/test-app/node_modules/.embroider/rewritten-packages/@ember-data/store.3e9c2e1b'
 @ ./tests/unit/models/common/user-test.ts 2:0-51 7:41-53
 @ ./assets/test.js 206:13-83

3 errors have detailed information that is not shown.
Use 'stats.errorDetails: true' resp. '--stats-error-details' to show it.

@NullVoxPopuli had a look at this in discord here and seems to think it is an embroider issue.

ef4 commented 1 year ago

Looking at the first error:

Module not found: Error: Can't resolve './json-api' in 'packages/my-addon/test-app/node_modules/.embroider/rewritten-packages/@ember-data/adapter.2dada623'

If you look inside that directory, we see that there is no ./json-api module present and there are no modules at all, just a package.json file which contains:

{
  "name":"@ember-data/adapter",
  "version":"5.1.1",
  "//":"This empty package was created by embroider. See https://github.com/embroider-build/embroider/blob/main/docs/empty-package-output.md"
}

That link is https://github.com/embroider-build/embroider/blob/main/docs/empty-package-output.md

It explains that you got here because this particular copy of @ember-data/adapter never actually got built within the classic build system, so there's no way for embroider to interpret what it contains.

In this case, that's happening because the copy of @ember-data/adapter that pnpm has provided to the addon is not actually the same copy that it provided to the app. That is arguably a pnpm bug. My suspicion is that it's being trigged by the fact that @ember-data/model and @ember-data/store have circular peer dependencies on each other, and that seems to trick pnpm's auto-install-peers into duplicating them.

This looks similar to the kind of problems that are fixed by using dependenciesMeta.*.injected, but that is not sufficient to fix it. Adding that to your repo moves the errors around slightly, but they still bottom out on @ember-data/store and @ember-data/model getting duplicated.

What does seem to avoid the problem for me is to not rely on auto-install-peers. If you add the addon's required peerDeps as normal devDeps in the test-app, the problem goes away.

ef4 commented 1 year ago

@ember-data/model and @ember-data/store have circular peer dependencies on each other, and that seems to trick pnpm's auto-install-peers into duplicating them.

I realized I might not have explained how this gets back to duplicated @ember-data/adapter. @ember-data/adapter has a peer dep on @ember-data/store, so if store gets duplicated, it's contagious an adapter also gets duplicated, because it "needs" to see a different peer when it's used in the addon vs when it's used in the app.

jeffsawatzky commented 1 year ago

@ef4 so the test-app, and any app that use the addon, will need to include the following peerDeps (from the addon here) as devDeps?

"@ember-data/adapter": "^5.1.1",
"@ember-data/model": "^5.1.1",
"@ember-data/store": "^5.1.1"

The test-app, and any app that uses the addon, is already including the ember-data meta package as a devDep, so adding these as well seems weird (duplicated dependencies) since ember-data already includes them as normal dependencies. I can remove the ember-data meta package from the test-app and all other apps, but then I run into other issues with missing dependencies that the ember-data meta package installs (you can see the list of them here). For example, one/more of the @ember-data/* packages needs ember-inflector but doesn't install it as a dependency and instead relies on the meta package to install it. Having my apps need to know what the dependencies of @ember-data/* packages are seems weird. Maybe this is an ember-data issue though with the way they specify dependencies?

Also, in some of the ember data packages, they have an external option in their rollup config, like this. This isn't in the default rollup config that is created with v2 addons, should it be? I have no idea what this is for...

ef4 commented 1 year ago

The test-app, and any app that uses the addon, is already including the ember-data meta package as a devDep, so adding these as well seems weird (duplicated dependencies) since ember-data already includes them as normal dependencies

Yeah, so the problem with that is that there's no reason to believe that ember-data's dependencies are available to the app. Historically npm and yarn would make them accidentally resolvable from the app, but in general it's just not true that you can always access your dependency's dependencies.

Also, auto-install-peers makes sure that the addon will automatically get copies of the peers that it needs if the app did not already provide them, but in that case only the addon can see them, the app cannot. The app can only see them if it explicitly lists them in dependencies or devDependencies.

jeffsawatzky commented 1 year ago

If you add the addon's required peerDeps as normal devDeps in the test-app, the problem goes away.

@ef4 I added the three peerDeps from my addon to the test-app, and I am still getting the same issue. I've update my repo with the latest changes. Am I supposed to add ALL of the dependencies that ember-data installs manually in my test-app?

ef4 commented 1 year ago

I only added those three. But I may have also deleted the lock file so everything could re-resolve. I also noticed that ember-data was installing at 5.1.0 when everything else was 5.1.1 so I adjusted that dep too.

On Wed, Jul 12, 2023 at 3:16 PM Jeff Sawatzky @.***> wrote:

If you add the addon's required peerDeps as normal devDeps in the test-app, the problem goes away.

I added the three peerDeps from my addon to the test-app, and I am still getting the same issue. I've update my repo with the latest changes. Am I supposed to add ALL https://github.com/emberjs/data/blob/main/packages/-ember-data/package.json#L22 of the dependencies that ember-data installs manually in my test-app?

— Reply to this email directly, view it on GitHub https://github.com/embroider-build/embroider/issues/1527#issuecomment-1633077046, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACN6MRL3UNJBWUUKBFA4ZLXP3ZYZANCNFSM6AAAAAA2GTNMHM . You are receiving this because you were mentioned.Message ID: @.***>

jeffsawatzky commented 1 year ago

@ef4 i did the same things as you. I adjusted ember-data to be 5.1.1, added those three devDeps, and deleted the lock file to re-resolve everything. You can see the diff in my latest commit in the example repo.

jeffsawatzky commented 1 year ago

Not sure if this helps, but here are the versions of things that I am on:

~ ❯ ember --version
ember-cli: 5.1.0
node: 18.16.1
os: darwin x64
~ ❯ pnpm --version
8.6.7

ef4 commented 1 year ago

After clarifying with @runspired what the relationship between the ember-data and @ember-data/* packages is right now: I think it's probably a mistake to mix them the way you do here. It's going to lead to duplications like this.

Both the addon and the app should use only ember-data and not the @ember-data/* packages.

The v2 addon build may have lead you to believe you need those real packages in order to import from them, but that is not the case. You can still do the backward-compatible thing by accessing them at runtime from the AMD loader. In a v2 addon you need to say so explicitly like:


// package.json
{
  "ember-addon": {
    "type": "addon",
    "version: 2,
    "externals": ["@ember-data/store", "@ember-data/model"]
  }
}

jeffsawatzky commented 1 year ago

The v2 addon build may have lead you to believe you need those real packages in order to import from them, but that is not the case.

Was the piece I was missing. Thanks! Is there documentation on the the externals option somewhere?

In my case I had to do the following in my v2 addon to make things work:

remove the peerDeps on the @ember-data/* packages
add a peerDep on the ember-data meta package
make changes to the rollup.config.mjs (to silence the rollup warnings) and package.json (to allow embroider to build properly) as follows:
```
// rollup.config.mjs
output: addon.output(),
```
external: [/@ember-data\/.*/],

plugins: [

// package.json "ember-addon": { "version": 2, "type": "addon", "main": "addon-main.cjs",

"externals": [
"@ember-data/adapter/json-api",
"@ember-data/model",
"@ember-data/store"

] }


In the test-app I was able to remove the `@ember-data/*` packages as well and only have the `ember-data` meta package as a peerDep.

With those changes I can build the v2 addon without issues, and ember test in the test-app passes.

Thanks so much for your help!

runspired commented 1 year ago

After reading this thread my suspicion is that this is an issue with peerDeps of peerDeps.

The isolation that gets created by pnpm doesn't traverse out ime. Peer-deps need redeclared at each level. So the peer-dep on Model means you need anything it declares too: iirc tracking, graph, and legacy-compat.

jeffsawatzky commented 1 year ago

@runspired maybe I'm not understanding something, but I think I gave your suggestion a try here.

I made my v2 addon use the @ember-data/* packages, instead of the ember-data meta package, as peerDeps, and then also added all the peerDeps of those as an explicit peerDep of my v2 addon.

I removed the excludes/exclude config from the rollup config and package json.

My test-app still uses the ember-data meta package as a devDep.

I deleted my node_modules folders, the pnpm lock file, and reinstalled. Then rebuilt the addon. Then ran ember test and I am once again getting the same "module not found" errors as before.

jeffsawatzky commented 1 year ago

@ef4 sorry to bother you again, but how would I resolve the following issue when I enable staticAddonTrees?

not ok 1 Chrome 114.0 - [undefined ms] - Global error: Uncaught Error: Could not find module `@ember-data/store/-private` imported from `(require)` at http://localhost:7357/assets/vendor.js, line 254
    ---
        browser log: |
            {"type":"error","text":"Uncaught Error: Could not find module `@ember-data/store/-private` imported from `(require)` at http://localhost:7357/assets/vendor.js, line 254\n","testContext":{}}
    ...
not ok 2 Chrome 114.0 - [undefined ms] - Global error: Uncaught Error: The tests file was not loaded. Make sure your tests index.html includes "assets/tests.js". at http://localhost:7357/1013287792301252/tests/index.html?hidepassed, line 50
    ---
        browser log: |
            {"type":"error","text":"Uncaught Error: The tests file was not loaded. Make sure your tests index.html includes \"assets/tests.js\". at http://localhost:7357/1013287792301252/tests/index.html?hidepassed, line 50\n","testContext":{"state":"complete"}}
    ...

1..2
# tests 2
# pass  0
# skip  0
# todo  0
# fail  2

I've updated my repo with the reproduction. When I disable staticAddonTrees the tests in the test-app pass fine, but when I enable it I get the above error.

embroider-build / embroider

`Module not found: Error: Can't resolve` errors with @ember-data imports in a v2 addon #1527