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

Cannot resolve debug on yarn 2.x.x #851

Closed
grapgrap opened this issue Aug 6, 2021 · 20 comments
Closed

Cannot resolve debug on yarn 2.x.x #851

grapgrap opened this issue Aug 6, 2021 · 20 comments
Labels
bug Something isn't working help wanted Extra attention is needed needs:discussion

Comments

@grapgrap
Copy link

grapgrap commented Aug 6, 2021

Describe the bug

Yarn cannot resolve 'debug' of msw

✖ 「wdm」: ERROR in ../../.yarn/unplugged/msw-npm-0.33.3-7287fb2021/node_modules/msw/lib/esm/index.js 10:0-31
Module not found: Error: Can't resolve 'debug' in '/Users/a202006041/workspace/toolbox/.yarn/unplugged/msw-npm-0.33.3-7287fb2021/node_modules/msw/lib/esm'
 @ ./src/mock/worker.ts 4:0-40 5:12-21 9:20-37
 @ ./src/mock/index.ts 38:19-37
 @ ./src/index.tsx 10:0-41 12:0-15

Environment

  • msw: 0.29.0
  • nodejs: 14.16.1
  • npm: 6.14.12
  • yarn: 2.4.2
  • webpack: 5.24.3

Please also provide your browser version.

To Reproduce

Steps to reproduce the behavior:

  1. Install msw@0.29.0 above.
  2. Run msw using webpack on yarn@2.x.x above

Expected behavior

When i tried with msw@0.28.2 and yarn@1.x.x each, It was ok.
I think module resolver cannot resolve pnp module that debug of msw external.

Additional Information

I tried to install debug and config it to peer dependency on .yarnrc.yml.
And then it works.

I did not use rollup well. I think msw should have debug to peer dependency.

// package.json
{
  ...
  devDependencies: {
    ...
    "debug": "^4.3.2",
    "msw": "^0.33.3",
  }
  ...
}

// .yarnrc.yml

packageExtensions:
  'msw@*':
    peerDependencies:
      'debug': '*'
@grapgrap grapgrap added the bug Something isn't working label Aug 6, 2021
@grapgrap grapgrap changed the title MSW cannot resolve debug on yarn 2.x.x Cannot resolve debug on yarn 2.x.x Aug 6, 2021
@kettanaito
Copy link
Member

Hey, @grapgrap. Thanks for reporting this.

Something seems to be off, but I don't have enough info to say on whose side it is.

I think msw should have debug to peer dependency.

MSW doesn't depend on debug directly, neither it uses that module. That module is likely installed by @mswjs/interceptors on which msw depends. The debug package is properly specified as a dependency in the interceptors package, so it must get installed when installing MSW.

@kettanaito kettanaito added the help wanted Extra attention is needed label Aug 6, 2021
@kettanaito
Copy link
Member

kettanaito commented Aug 6, 2021

I can also recommend raising an issue in the Yarn's repo to see what their maintainers can recommend. At the first glance, I don't see what may be wrong with how we specify the dependencies. The fact that this is only reproduced on yarn@2 may suggest there's an issue with Yarn.

@merceyz
Copy link

merceyz commented Aug 6, 2021

MSW doesn't depend on debug directly, neither it uses that module. That module is likely installed by @mswjs/interceptors on which msw depends. The debug package is properly specified as a dependency in the interceptors package, so it must get installed when installing MSW.

It does actually import it directly https://cdn.jsdelivr.net/npm/msw@0.33.3/lib/esm/index.js, most likely caused by

external: ['debug'],

The fact that this is only reproduced on yarn@2 may suggest there's an issue with Yarn.

Nope, Yarn PnP is just really strict about undeclared dependencies

@kettanaito
Copy link
Member

It does actually import it directly https://cdn.jsdelivr.net/npm/msw@0.33.3/lib/esm/index.js

That import is correct: the debug package is whitelisted from bundling and specified as an external dependency. We shouldn't bundle debug because MSW code can execute in both browser and Node.js, but the bundler itself is a Node.js process, so, when bundled, debug thinks it's being used in Node.js, resulting in a Node.js-specific code being bundled. That code freely relies on Node.js standard library, which breaks the browser runtime of MSW.

I believe that specifying it as an external dependency is the primary use-case for externals: it gets required on runtime, then debug decides which environment it executes in, and uses the respective code.

I'm open to refactoring this.

@merceyz
Copy link

merceyz commented Aug 6, 2021

Yeah that's fine, but it means msw needs to declare debug as a dependency
https://yarnpkg.com/advanced/rulebook#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies

@kettanaito
Copy link
Member

kettanaito commented Aug 6, 2021

That sounds unapparent to me. The debug import ends up in the built bundle because of a dependency (interceptors library). Specifying debug as a direct msw dependency implies that MSW relies on it, when, in fact, it doesn't, it's the interceptors library that relies on it (we shouldn't analyze the compiled code to make such assumptions, in my opinion).

This sounds like the debug bundling issue and not the dependency issue.

@grapgrap
Copy link
Author

grapgrap commented Aug 7, 2021

Thanks for your comments.

If msw really did not depend on debug, You can remove debug from rollup config?
It seems that the debug used by @mswjs/interceptors is being removed by the external option written in the rollup config.

It will help me to understand the misunderstandings caused by my poor English.

msw
|__ msw/interceptors
       |__ debug --> be extracted by rollup?

@kettanaito
Copy link
Member

If debug is removed from externals it will be bundled alongside @mswjs/interceptors (that dependency is rightfully bundled). When bundled, as I've described above, debug thinks it's being evaluated in a Node.js environment (Rollup is a Node.js process), so only the Node-specific code is left from debug. When that code is present in a browser bundle of MSW, it causes errors because it relies on the standard Node.js library.

@grapgrap
Copy link
Author

grapgrap commented Aug 9, 2021

I understand your comments.
But, I was curious as to why debug is being imported in the build results.
So, i found the code in build result about msw.

// lib/index.js

import require$$3 from 'debug';
...
var debug$1 = require$$3('fetch');
...
debug$1('isomorphic request', isoRequest);
observer.emit('request', isoRequest);
debug$1('awaiting for the mocked response...');

And I searched the string, awaiting for the mocked response....
And then i could found the code in this file on @mswjs/interceptors.

The code use in this file on msw.

I think this part is the cause that try to import debug on msw

ps. Sorry for the slow reply.

@kettanaito
Copy link
Member

kettanaito commented Aug 9, 2021

No worries, @grapgrap. Your debugging is correct: the debug import ends up in the MSW bundle because it bundles mswjs/interceptors as a dependency. Here's a dependency tree so that this moment is clear to everybody:

msw
  - @mswjs/interceptors
    - debug

Here are the summarized statements:

  • MSW does not depend on debug.
  • MSW depends on @mswjs/interceptors.
  • @mswjs/interceptors depends on debug.

Once again, in a proper world, debug module will be processed and included in the MSW build when it bundles the interceptors library. That cannot be done because debug would then think it's being used in a Node.js environment during bundling by Rollup, stripping away all the browser-related code.

It may feel natural to list debug as MSW's dependency then because the end code imports debug, but this is not semantic: MSW does not use debug directly. Let's say the interceptors library drops debug in the next release. That would leave MSW with a debug dependency it forces all users to install, which is never used.

@merceyz
Copy link

merceyz commented Aug 9, 2021

No matter how the import ends up in the code, the fact is that it's there and needs to be declared as a dependency, please read the docs I linked to in #851 (comment)

Let's say the interceptors library drops debug in the next release. That would leave MSW with a debug dependency it forces all users to install, which is never used.

Lets say it does drop it and debug isn't in the dependency tree anymore, now users will get an error that debug can't be found, just like this issue.

@kettanaito
Copy link
Member

kettanaito commented Aug 9, 2021

What I think is the solution here is to figure out how to properly bundle debug so, when bundled, it doesn't evaluate and remove its code, thinking it's being run in Node.js. It may be an issue with how debug is bundled by the interceptors library.

@kettanaito
Copy link
Member

kettanaito commented Aug 9, 2021

Lets say it does drop it and debug isn't in the dependency tree anymore, now users will get an error that debug can't be found.

They will not get that error with the current setup. If the debug is dropped, it won't resurface in MSW, and it won't be handled as external, appending the import statement. MSW would stop relying on debug if it's being dropped by the interceptors.

@merceyz, my statement above still stands: I don't think this is a dependency specification issue. It's a bundling issue. If the debug package can be properly bundled, preserving this logical fork, then we can remove it from externals in MSW and have it bundled alongside the interceptors library without issues.

@grapgrap
Copy link
Author

If you remove (or replace the logical fork) debug from interceptor, It seem to be ok.
Thanks a lot your help. 🙏

@kettanaito
Copy link
Member

I've opened an issue (debug-js/debug#836) in debug to suggest the right approach.

@grapgrap
Copy link
Author

@kettanaito Thanks you! I follow the issue 👍

@kettanaito
Copy link
Member

While we were discussing this, I've noticed that the interceptors package (that relies on debug) doesn't in fact have a build step. That may be the issue, causing the debug dependency to be evaluated instead of bundled as-is.

Historically, I recall there was an issue when building interceptors, which caused some of them to fail in certain environments (don't remember the exact context). Introducing a build step should be done with caution.

@patrickmcdougle-okta
Copy link
Contributor

Most of this packages dependencies are bundled into the build lib/ folder using rollup and thus don't actually need to be dependencies, but rather should be devDependencies IMO.

@kettanaito
Copy link
Member

You've raised a valid point, @patrickmcdougle-okta, that some of the dependencies should be treated as external. That'd help with bug-fixes propagation and make the dependency tree more transparent.

As for the other dependencies, I believe some must be bundled. It'd be nice to review the entire dependency tree and decide the pain points to fix. Contributions are always welcome!

@patrickmcdougle-okta
Copy link
Contributor

Digging further at this, we shouldn't externalize debug in the build if we also don't mark it as a dependency. We're effectively telling the system "don't bundle debug please, because I promise it will exist at run time" but then we don't enforce that it exists at runtime through a dependency. We can't have it both ways, either we bundle debug OR list it as a dependency. Right now we're not doing either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed needs:discussion
Projects
None yet
Development

No branches or pull requests

4 participants