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

Fix getPackageJson problem for pnpm #13399

Closed
wants to merge 2 commits into from
Closed

Conversation

taai
Copy link

@taai taai commented May 11, 2022

When using pnpm, the Quasar CLI with using function getPackageJson() fails to resolve vite package (to get version number from its package.json file) because of how pnpm structures the node_modules.

The problem is in file app-vite/lib/helpers/banner-global.js. The getPackageJson() function could not resolve the vite package and returned undefined...

This fix makes the resolver to look first in the specified directory and then in default paths instead of just in the specified directory. Then it works also with pnpm out of the box.

The error:

> quasar dev

 .d88888b.
d88P" "Y88b
888     888
888     888 888  888  8888b.  .d8888b   8888b.  888d888
888     888 888  888     "88b 88K          "88b 888P"
888 Y8b 888 888  888 .d888888 "Y8888b. .d888888 888
Y88b.Y8b88P Y88b 888 888  888      X88 888  888 888
 "Y888888"   "Y88888 "Y888888  88888P' "Y888888 888
       Y8b


 App • ⚠️  ️️Setting port to closest one available: 9001

/app/node_modules/.pnpm/@quasar+app-vite@1.0.0-beta.14_aonw7kxj5rrzambm5jgx5za7q4/node_modules/@quasar/app-vite/lib/helpers/banner-global.js:6
const viteVersion = getPackageJson('vite').version
                                          ^

TypeError: Cannot read properties of undefined (reading 'version')
    at Object.<anonymous> (/app/node_modules/.pnpm/@quasar+app-vite@1.0.0-beta.14_aonw7kxj5rrzambm5jgx5za7q4/node_modules/@quasar/app-vite/lib/helpers/banner-global.js:6:43)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/app/node_modules/.pnpm/@quasar+app-vite@1.0.0-beta.14_aonw7kxj5rrzambm5jgx5za7q4/node_modules/@quasar/app-vite/lib/helpers/print-dev-banner.js:5:64)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Documentation
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

The PR fulfills these requirements:

  • It's submitted to the dev branch (or v[X] branch)
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on a Cordova (iOS, Android) app
  • It's been tested on an Electron app
  • Any necessary documentation has been added or updated in the docs or explained in the PR's description.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to start a new feature discussion first and wait for approval before working on it)

Other information:

taai added 2 commits May 11, 2022 16:41
When using pnpm without shameful-hoist setting, the Quasar CLI failed to resolve vite package because of how pnpm structures the node_modules
@taai
Copy link
Author

taai commented May 16, 2022

I think I found all places where the code with require.resolve is trying to resolve packages from node_modules directory to get some file from some package. The rest places using require.resolve, I think, should stay as is, because these in these code places the relative files are being resolved.
I haven't tested this in every possible way (I have tested npm and pnpm), but I believe that this will not break anything (all are wrapped in try-catch blocks already).

@yusufkandemir
Copy link
Member

yusufkandemir commented Jun 5, 2022

After lots of digging in and different tests, I found out that this is definitely not what we want. Vite is a direct dependency of app-vite, but it was using getPackageJson helper, which is meant to be used for host packages such as Quasar, Capacitor, etc. that are inside project root, src-* folders, etc. I implemented a fix in #13609 for this. I also took the time to simplify and clarify those helpers to avoid further confusion or errors(#13608). Thanks for your time and investigation.

With module.paths added to the paths option, it starts resolving some packages in any possible case. One example would be store provider logic, we try to resolve Vuex to see if the user installed it, we use Pinia otherwise. But, with module.paths, Node resolves Vuex from @quasar/app-*'s devDependencies even if the user didn't install it, so it becomes impossible to not use Vuex.

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

Successfully merging this pull request may close these issues.

None yet

2 participants