happo / happo.io

Happo is a cross-browser screenshot testing service
https://happo.io
MIT License
196 stars 25 forks source link

Error running in yarn due to undeclared dependency #268

Open dobesv opened 7 months ago

dobesv commented 7 months ago

Yarn is strict about using dependencies that are not declared in package.json - if a file imports or requires a module it must be declared in that file's package.json. This differs from npm where a module could use the dependencies of its dependencies freely.

 yarn run happo dev
Starting: [main] Searching for happo test files... 
✗ [main] Searching for happo test files...: Error: happo.io tried to access react-dom, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

Required package: react-dom
Required by: happo.io@virtual:0b8541be94c07b03e28e04a869f424dd9b9352f559e63e47625f947ec9ee14213658be2582b051a033b49b2f2beeedb9f8fde576b107aac3aa921ebdc44b4415#npm:9.1.2
ljharb commented 7 months ago

note that it's not "legacy" environments, it's "the standard environment" (ie, npm).

trotzig commented 7 months ago

Thanks for the report @dobesv!

This is an unfortunate effect of trying to keep the library free from a direct dependency on react (and react-dom). We could have react-dom listed as a peerDependency but that would cause everyone to pull down React even if they aren't using it.

I think the "right" solution here is to make the react dependency a plugin of some sort, or force the user to inject it somehow. This block is where the import happens: https://github.com/happo/happo.io/blob/0cc7b7bc650c43037d12d8f7ffe5abdadcca010a/src/createDynamicEntryPoint.js#L53

Do you have a workaround currently? I looked around the wwws and found that you might be able to add this to your .yarnrc.yml:

packageExtensions:
  happo.io@*:
    dependencies:
      react-dom: "*"
      react: "*"

From here: https://github.com/styled-components/styled-components/issues/3082#issuecomment-711163684

dobesv commented 7 months ago

Yeah it can be worked around changing .yarnrc but obviously that won't be friendly to new users.

There are two resolutions to this issue that I know of:

  1. Add an optional peerDependency - e.g. add it to peerDependencies and also mark it optional in peerDependenciesMeta. This allows you to use it if you want to withoutwarnings if it is not present. I'm not 100% if/how npm supports this one though.
  2. Use createRequire from https://nodejs.org/api/module.html#modulecreaterequirefilename to dynamically import the module from within the target package's import context, in this case it is treated as if you are a file in that target package for purposes of module resolution

Which you choose depends on the details of what you are up to. My guess is that #1 should work OK.

ljharb commented 7 months ago

It depends on what node/npm versions you support; npm certainly supports peerDependenciesMeta, but versions of npm prior to that will treat the peer dep as required.