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

Module not found: Error: Can't resolve '@ember-data/tracking/-private' #8296

Closed
Turbo87 opened this issue Nov 7, 2022 · 15 comments · Fixed by #8314
Closed

Module not found: Error: Can't resolve '@ember-data/tracking/-private' #8296

Turbo87 opened this issue Nov 7, 2022 · 15 comments · Fixed by #8314
Labels
upstream Upstream Dependency Fix Required

Comments

@Turbo87
Copy link
Member

Turbo87 commented Nov 7, 2022

https://github.com/rust-lang/crates.io/actions/runs/3408161209/jobs/5668483713

Reproduction

see rust-lang/crates.io#5277

Description

It looks like the most recent v4.8 release can't be built with embroider anymore due to the error below:

stack: ModuleNotFoundError: Module not found: Error: Can't resolve '@ember-data/tracking/-private' in '$TMPDIR/embroider/c37dd7/node_modules/.pnpm/@[email protected]_t4t7pte3g4axxazuc5bgp4ddgu/node_modules/@ember-data/model/-private/record-state.js'
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/webpack/lib/Compilation.js:2016:28
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/webpack/lib/NormalModuleFactory.js:798:13
    at eval (eval at create (/home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:10:1)
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/webpack/lib/NormalModuleFactory.js:270:22
    at eval (eval at create (/home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:9:1)
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/webpack/lib/NormalModuleFactory.js:434:22
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/webpack/lib/NormalModuleFactory.js:116:11
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/webpack/lib/NormalModuleFactory.js:670:25
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/webpack/lib/NormalModuleFactory.js:855:8
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/webpack/lib/NormalModuleFactory.js:975:5
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/neo-async/async.js:6883:13
    at /home/runner/work/crates.io/crates.io/node_modules/.pnpm/[email protected]/node_modules/webpack/lib/NormalModuleFactory.js:958:45
@runspired
Copy link
Contributor

runspired commented Nov 8, 2022

@mkszepp I don't think these are related at all, I'm fairly sure embroider/consuming applications need to add an adapter since the tracking package is a v1-addon with a custom treeForAddon

@runspired runspired added the upstream Upstream Dependency Fix Required label Nov 8, 2022
@mkszepp
Copy link
Contributor

mkszepp commented Nov 11, 2022

@runspired is there any workaround?

I have tried to create an adapter, the build works fine, but in browser i got this error: Expected store.createRecordDataFor to be implemented but it wasn't

ember-cli-build.js:

'use strict';

const EmberApp = require('ember-cli/lib/broccoli/ember-app');
const { V1Addon } = require('@embroider/compat');
const buildFunnel = require('broccoli-funnel');
const mergeTrees = require('broccoli-merge-trees');

class EmberDataCompatAdapter extends V1Addon {
  get v2Tree() {
    return mergeTrees([
      super.v2Tree,
      buildFunnel(this.rootTree, {
        include: ['dist/*.js'],
      }),
    ]);
  }
}

module.exports = function (defaults) {
  const app = new EmberApp(defaults, {
    // Add options here
  });

  // Use `app.import` to add additional libraries to the generated
  // output files.
  //
  // If you need to use different assets in different
  // environments, specify an object as the first parameter. That
  // object's keys should be the environment name and the values
  // should be the asset to use in that environment.
  //
  // If the library that you are including contains AMD or ES6
  // modules that you would like to import into your application
  // please specify an object with the list of modules as keys
  // along with the exports of each module as its value.

  const { Webpack } = require('@embroider/webpack');
  return require('@embroider/compat').compatBuild(app, Webpack, {
    skipBabel: [
      {
        package: 'qunit',
      },
    ],
    compatAdapters: new Map([['@ember-data/tracking', EmberDataCompatAdapter]]),
  });
};

grafik

@runspired
Copy link
Contributor

@mkszepp I think if you specify compat adapters you end up having to specify all the data packages because you override the default Map which provides the compat adapters for these iirc

@runspired
Copy link
Contributor

I have an idea of how to make these "v1 addons masquerading as v2 addons" safer for embroider, will push what I suspect is a fix in a moment. Should mean no compat-adapter is necessary.

Embroider doesn't run an addon's treeForAddon (it honestly should when in compat mode, but alas). This has historically been the reason I don't recommend using embroider+ember-data until ember-data ships as v2 addons, because by doing this you opt into a ton of slow-paths and bloat.

The fix TL;DR is that I'll write both a dist and an addon directory during build, so that both trees are present to be consumed by whichever means ends up being used (ember-cli, embroider, with or without v2 addon mode)

@runspired
Copy link
Contributor

looks like after testing this was not enough, embroider apparently can't see the tracking package from within the other ember-data packages when compiling. Unsure why.

@runspired
Copy link
Contributor

Probably all of the EmberData addon compat-adapters should just do this:

import V1Addon from '../../v1-addon';

export default class VerticalCollection extends V1Addon {
  // `@html-next/vertical-collection` does some custom Babel stuff, so we'll let it do it's own thing
  customizes(...names: string[]) {
    return super.customizes(...names.filter(n => n !== 'treeForAddon'));
  }
}

https://github.com/embroider-build/embroider/blob/main/packages/compat/src/compat-adapters/%40html-next/vertical-collection.ts

@mkszepp
Copy link
Contributor

mkszepp commented Nov 15, 2022

@runspired Embroider brings already this adapter for EmberData begining with embroider v1.2

export class EmberDataBase extends V1Addon {
  // May of the ember-data packages use rollup to try to hide their internal
  // structure. This is fragile and it breaks under embroider, and they should
  // really move this kind of "build-within-a-build" to prepublish time.
  //
  // This disables any custom implementation of `treeForAddon`. The stock
  // behavior is correct.
  customizes(...names: string[]) {
    return super.customizes(...names.filter(n => n !== 'treeForAddon'));
  }
}

https://github.com/embroider-build/embroider/blob/3b0dd9176c8c052ab2a02c88cacbe1f127dd2f74/packages/compat/src/compat-adapters/ember-data.ts#L7-L17

There was created a subfolder with adaperts for some EmberData addons https://github.com/embroider-build/embroider/tree/main/packages/compat/src/compat-adapters/%40ember-data

This adapters are working with ember data < v4.8.x.

I have also tried use this adapter for tracking, but it doen't work.
To solve this error, there was necessary to copy all files from dist folder to root, but i was running in the next error, that the const HAS_RECORD_DATA_PACKAGE is now always false (all other package const are true https://github.com/emberjs/data/blob/master/packages/private-build-infra/addon/index.ts) and at this point idk how to fix it.

Maybe with your pull request, there is not anymore necessary to copy the dist folder, because its copied in an other way.

'use strict';

const EmberApp = require('ember-cli/lib/broccoli/ember-app');
const { EmberDataBase } = require('@embroider/compat/src/compat-adapters/ember-data');
const buildFunnel = require('broccoli-funnel');
const mergeTrees = require('broccoli-merge-trees');

class EmberDataCompatAdapter extends EmberDataBase {
  get v2Tree() {
    return mergeTrees([
      super.v2Tree,
      buildFunnel(this.rootTree, {
        include: ['dist/*'],
      }),
    ]);
  }
}

module.exports = function (defaults) {
  const app = new EmberApp(defaults, {
    // Add options here
  });

  // Use `app.import` to add additional libraries to the generated
  // output files.
  //
  // If you need to use different assets in different
  // environments, specify an object as the first parameter. That
  // object's keys should be the environment name and the values
  // should be the asset to use in that environment.
  //
  // If the library that you are including contains AMD or ES6
  // modules that you would like to import into your application
  // please specify an object with the list of modules as keys
  // along with the exports of each module as its value.

  const { Webpack } = require('@embroider/webpack');
  return require('@embroider/compat').compatBuild(app, Webpack, {
    skipBabel: [
      {
        package: 'qunit',
      },
    ],
    compatAdapters: new Map([['@ember-data/tracking', EmberDataCompatAdapter]]),
  });
};

@runspired
Copy link
Contributor

Yeah I realized later they need to do the opposite of that. I got it mostly working replacing all the adapters with ones that just let EmberData do it's thing normally. Only issue remaining seems to be the version module, unsure why it's giving me grief of the addon tree is properly running 🤔

That said think the main issue is just a bug in embroider in which it creates artificial node_modules trees inside the tmpdir that don't work for pnpm

@ef4
Copy link
Contributor

ef4 commented Nov 23, 2022

The problem here is that v1 addons don't support the exports key in package.json. If you want to use that, you need to go to v2.

I confirmed that deleting exports from @ember-data/tracking/package.json fixes the build crash.

@Turbo87
Copy link
Member Author

Turbo87 commented Dec 3, 2022

@runspired rust-lang/crates.io#5277 now shows "Expected store.createRecordDataFor to be implemented but it wasn't" with v4.9.1 which, according to #8296 (comment), appears to be related to this?

I guess we should reopen the issue? 😅

@runspired
Copy link
Contributor

@Turbo87 probably a new issue, as we only saw that when trying to manually replace the compat adapters. may be related to ember-source tbh as we added test coverage that is passing for embroider. It fails (though with a different error) if we try to bump to 4.9 ember-source.

@mkszepp
Copy link
Contributor

mkszepp commented Dec 5, 2022

@runspired this bug is still present in the v4.8.4, which is now "LTS"... :( I have also seen that other bugs, which were already fixed, were not fixed in the LTS :( . The difference between 4.8.3 and 4.8.4 is only the version.... Is there planned to get this fixes also in 4.8.x or shoud we stay on the 4.4 since 4.12 is relesed as LTS?

@runspired
Copy link
Contributor

The difference between 4.8.3 and 4.8.4 is only the version

yes, this is an artifact of how npm works. To publish an updated dist-tag you need a new version. So promoting it to lts bumps the version.

Is there planned to get this fixes also in 4.8.x

yes, I haven’t had time to port the work backwards but if you’d like that work faster and don’t want to use 4.9 then opening a PR cherry-picking those fixes to the lts-4-8 branch would speed this up.

@Turbo87
Copy link
Member Author

Turbo87 commented Dec 5, 2022

To publish an updated dist-tag you need a new version.

you can also use something like npm dist-tag 4.8.3 lts for this (see https://docs.npmjs.com/cli/v9/commands/npm-dist-tag)

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

Successfully merging a pull request may close this issue.

4 participants