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

False positive in ModuleScopePlugin with yarn2 "workspace:*" packages #10508

Open
Xerkus opened this issue Feb 5, 2021 · 8 comments · May be fixed by #11237
Open

False positive in ModuleScopePlugin with yarn2 "workspace:*" packages #10508

Xerkus opened this issue Feb 5, 2021 · 8 comments · May be fixed by #11237

Comments

@Xerkus
Copy link

Xerkus commented Feb 5, 2021

Describe the bug

ModuleScopePlugin incorrectly detects import from package in yarn 2 pnp workspace as relative path.

Module not found: You attempted to import /snip/packages/assets/Logo-BlueOrange.svg which falls outside of the project src/ directory. Relative imports outside of src/ are not supported.

// package.json
"dependencies": {
    "@my/assets": "workspace:*",
}
// component
import Logo from '@my/assets/Logo-BlueOrange.svg';

Same behavior is with importing .ts, so not svg specific.

Did you try recovering your dependencies?

Which terms did you search for in User Guide?

Environment

react-dev-utils@npm:11.0.2
   ├─ Version: 11.0.2
   │
   ├─ Dependents
   │  ├─ react-scripts@npm:4.0.2
   │  └─ react-scripts@npm:4.0.2 [675a5]
   │
   └─ Dependencies
      ├─ @babel/code-frame@npm:7.10.4 → npm:7.10.4
      ├─ address@npm:1.1.2 → npm:1.1.2
      ├─ browserslist@npm:4.14.2 → npm:4.14.2
      ├─ chalk@npm:2.4.2 → npm:2.4.2
      ├─ cross-spawn@npm:7.0.3 → npm:7.0.3
      ├─ detect-port-alt@npm:1.1.6 → npm:1.1.6
      ├─ escape-string-regexp@npm:2.0.0 → npm:2.0.0
      ├─ filesize@npm:6.1.0 → npm:6.1.0
      ├─ find-up@npm:4.1.0 → npm:4.1.0
      ├─ global-modules@npm:2.0.0 → npm:2.0.0
      ├─ globby@npm:11.0.1 → npm:11.0.1
      ├─ gzip-size@npm:5.1.1 → npm:5.1.1
      ├─ immer@npm:7.0.9 → npm:7.0.9
      ├─ is-root@npm:2.1.0 → npm:2.1.0
      ├─ loader-utils@npm:2.0.0 → npm:2.0.0
      ├─ open@npm:^7.0.2 → npm:7.4.0
      ├─ pkg-up@npm:3.1.0 → npm:3.1.0
      ├─ prompts@npm:2.4.0 → npm:2.4.0
      ├─ react-error-overlay@npm:^6.0.9 → npm:6.0.9
      ├─ recursive-readdir@npm:2.2.2 → npm:2.2.2
      ├─ shell-quote@npm:1.7.2 → npm:1.7.2
      ├─ strip-ansi@npm:6.0.0 → npm:6.0.0
      ├─ text-table@npm:0.2.0 → npm:0.2.0
      └─ fork-ts-checker-webpack-plugin@npm:4.1.6 → npm:4.1.6 [10d52]

Steps to reproduce

Setup CRA in yarn 2 workspace

  1. Create test directory
    mkdir yarnws && cd yarnws
  2. Set local yarn version to 2.x
    yarn set version berry && yarn set version latest
  3. Verify version
    yarn --version > 2.4.0
  4. Init yarn workspace
    yarn init -w
  5. Create react app
    (cd packages/ && yarn create react-app --template typescript test-cra)
  6. Remove bits not used in workspace install
    rm -rf packages/test-cra/{.pnp.js,.yarn,yarn.lock}
  7. Install dependencies
    yarn up
  8. Yarn needs those as direct dependencies:
    yarn workspace test-cra add eslint-config-react-app react-refresh
  9. Verify it works
    yarn workspace test-cra start
  10. Create another package:
    (mkdir packages/testpkg && cd packages/testpkg && yarn init)
  11. Add package to yarn app
    yarn workspace test-cra add "testpkg@workspace:*"
  12. Make ts file in the testpkg package and try to use it in react app as proper module import, eg import { test } from 'testpkg/test'

Expected behavior

ModuleScopePlugin does not complain and does not prevent compilation

Actual behavior

Module not found: You attempted to import /home/Xerkus/workspace/yarnws/packages/testpkg/test.ts which falls outside of the project src/ directory. Relative imports outside of src/ are not supported.

Reproducible demo

https://github.com/Xerkus/cra-10508

@Xerkus
Copy link
Author

Xerkus commented Feb 5, 2021

Implication here is quite serious: CRA app in yarn 2 workspace with pnp can't be split into multiple packages in the same workspace due to this check.

@charrondev
Copy link
Contributor

This seems similar to this issue that was previously closed #8757 and not resolved.

@charrondev
Copy link
Contributor

Also related #9226

@Xerkus
Copy link
Author

Xerkus commented Mar 8, 2021

For yarn this can be resolved using yarn specific lookup in the plugin to see if it is a module include https://yarnpkg.com/advanced/pnpapi/

@Xerkus
Copy link
Author

Xerkus commented Apr 29, 2021

I have no clue what I am doing, but using yarn patch protocol I made it work, I think

diff --git a/ModuleScopePlugin.js b/ModuleScopePlugin.js
index e84d2b38aabbfc8e28515859417ae9652b711b57..8d450373ceebfd8ac747f6165f2d8822900d8b26 100644
--- a/ModuleScopePlugin.js
+++ b/ModuleScopePlugin.js
@@ -35,6 +35,12 @@ class ModuleScopePlugin {
         ) {
           return callback();
         }
+        if (process.versions.pnp) {
+          const {findPackageLocator} = require('pnpapi');
+          if (findPackageLocator(request.__innerRequest_request) !== null) {
+            return callback();
+          }
+        }
         // Resolve the issuer from our appSrc and make sure it's one of our files
         // Maybe an indexOf === 0 would be better?
         if (

@russell-dot-js
Copy link

@Xerkus I tested your changes and they work -- put up a PR with your changes here #11237
Hopefully this gets merged, but I'm hosting a version you can use as a resolution in the meantime

@Xerkus
Copy link
Author

Xerkus commented Aug 7, 2021

The workaround is not a complete check and most likely not even a part of a proper check. The idea was to see if file can be resolved to be belonging to one of the packages declared as a dependency, but that does not guarantee that it is required via dependency and not directly as a path.
I assume that is what it does but I don't actually know.

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants