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

Doesn't work with yarn berry PnP mode #41

Open
dobesv opened this issue Apr 25, 2023 · 3 comments
Open

Doesn't work with yarn berry PnP mode #41

dobesv opened this issue Apr 25, 2023 · 3 comments
Labels
On Hold The issue blocked by something P2 Important. Nice to have improvements and optimizations.

Comments

@dobesv
Copy link

dobesv commented Apr 25, 2023

I tried migrating to this but I get an error when it tries to resolve the path to a css file:

 @dr.pogodin/babel-plugin-react-css-modules tried to access @draft-js-plugins/alignment, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

Required package: @draft-js-plugins/alignment (via "@draft-js-plugins/alignment/lib/plugin.css")
Required by: @dr.pogodin/babel-plugin-react-css-modules@virtual:978be09f033f9d1d87a6ae470cf41768305eae1692596bdc022dee76fc3671ae951ebf07cf433ace646b0e1c79a9e1b2be00e15f7afac12075ecdcf897b30d3a#npm:6.9.4 (via /home/dobes/projects/formative/app/.yarn/__virtual__/@dr.pogodin-babel-plugin-react-css-modules-virtual-cbe7ecd98b/0/cache/@dr.pogodin-babel-plugin-react-css-modules-npm-6.9.4-458b93ef69-ad14c19a11.zip/node_modules/@dr.pogodin/babel-plugin-react-css-modules/dist/)

In Yarn PnP environments you cannot use require directly to access the dependencies of the packages that use your package. You have to do something like:

import { createRequire } from 'node:module';

// Sometime later

function resolveUsingImportLogic(path, projectDir = process.cwd()) {
  const targetRequire = createRequire(
    path.resolve(projectDir, 'package.json'),
  );
  const resolvedPath = targetRequire.resolve(thingYouWantToResolve);
}
@birdofpreyru
Copy link
Owner

Well... your issue seems to be somewhat beyond my NodeJS experience — I use NPM rather than Yarn; it is the first time I hear about the Yarn PnP environment; and in my head dynamic require() of a module is legit if the module can be found by the standard NodeJS resolution rules.

Thus, not surprised it does not work. I'll need to find time to educate myself about it, and to figure out what should be done about it. Also, at the first glance to your code snippet, my first doubt is that I imagine many projects may have package.json not located in the process.cwd() folder, so doing something like this probably would be a huge breaking change for existing projects relying on the normal NodeJS / NPM environment.

@dobesv
Copy link
Author

dobesv commented Apr 26, 2023

I imagine many projects may have package.json not located in the process.cwd() folder

Sorry, I provided a bad example there.

The path passed to createRequire is the path of a file that is supposedly doing the import and is used to run the standard NodeJS resolution rules from that location instead of from the file that you are currently in (in the plugin).
This is probably a better example:

import { createRequire } from 'node:module';

const getTargetResourcePath = (importedPath: string, stats: *) => {
  const targetFileDirectoryPath = dirname(stats.file.opts.filename);

  if (importedPath.startsWith('.')) {
    return resolve(targetFileDirectoryPath, importedPath);
  }
  const targetRequire = createRequire(stats.file.opts.filename);
  return targetRequire.resolve(importedPath);
};

Yarn Plug'n'Play adds a number of improvements to the node modules resolution system for better reliability & performance. You can read more here: https://yarnpkg.com/features/pnp

However, it does apply stricter rules when importing things - modules are not allowed to import things that are not declared in their own closest package.json. So when using require.resolve you have to use the above technique so Yarn recognizes which package the import is coming from and can apply the rule as approporiate for that package instead (the one being processed using babel, rather than the plugin code).

Note that even if yarn pnp is not being used, using createRequire to resolve relative to the specified module should be better in general as it will resolve the module path they would have gotten from that location.

Consider this somewhat contrived file tree:

node_modules
node_modules/babel
node_modules/@dr.pogodin/babel-plugin-react-css-modules
packages/a/node_modules/great-styles/a.css
packages/a/src/source-code.tsx

if packages/a/src/source-code.tsx imports great-styles/a.css your plugin won't find it because it would be looking for node modules up the wrong branch of the file tree.

@birdofpreyru birdofpreyru added the P2 Important. Nice to have improvements and optimizations. label Apr 26, 2023
@birdofpreyru birdofpreyru added the On Hold The issue blocked by something label Aug 14, 2023
@birdofpreyru
Copy link
Owner

For the record, I'll copy paste here a relevant note from the release v6.11.0.

Injecting an absolute path to a module into a compiled code is a terrible idea, as it implies that the compiled code will not work if its runtime environment is different from the build environment (e.g. modules have been moved around after the build, or the code is build on one system and used on another, etc.). I had a glance at pnpm documentation, and it seems to me that to avoid the problem PR #42 intended to solve, one just have to correctly configure his pnpm project, maybe to add some pnpm-specific configuration to this plugin as well. Not something I personally will spend my time on investigating further, unless somebody brings in funds to cover it. If you can't figure it out, neither able to bring in the funding for me to figure it out, I just suggest using the normal npm — it might be slower and less disk-space efficient, but it turns out it comes out with an easier developer experience :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Hold The issue blocked by something P2 Important. Nice to have improvements and optimizations.
Projects
None yet
Development

No branches or pull requests

2 participants