fix: use globbySync to resolve PNPM global node_modules paths#692
fix: use globbySync to resolve PNPM global node_modules paths#692mshima merged 2 commits intoyeoman:mainfrom phishbacon:fix_pnpm_global_node_module_path_resolution
Conversation
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for hopping onto a fix! I confirmed locally that e.g. /Users/josh/Library/pnpm/global/5/node_modules does include the generator after pnpm i -g generator-office.
This area is finnicky and new, so we should definitely hear from >=1 of @mshima and @UlisesGascon for review.
src/module-lookup.ts
Outdated
| onlyDirectories: true, | ||
| suppressErrors: true, | ||
| absolute: true, | ||
| } as GlobbyOptions).map(modulesPath => resolve(pnpmHome, modulesPath)), |
There was a problem hiding this comment.
[Cleanup] Nit: unlike the other uses, this one doesn't need an assertion:
| } as GlobbyOptions).map(modulesPath => resolve(pnpmHome, modulesPath)), | |
| }).map(modulesPath => resolve(pnpmHome, modulesPath)), |
There was a problem hiding this comment.
awesome thanks for the review. I removed the assertion. I want to add that I've tested on ubuntu and windows
after installing generator-office globally with pnpm and running yo with yeoman-environment linked with my local changes.
on windows:
PS C:\Users\christian\development\yo> echo $env:PNPM_HOME
C:\Users\christian\AppData\Local\pnpm
PS C:\Users\christian\development\yo> node .\lib\cli.js --generators
Available Generators:
office
PS C:\Users\christian\development\yo> pnpm list -g
Legend: production dependency, optional only, dev only
C:\Users\christian\AppData\Local\pnpm\global\5
dependencies:
generator-office 3.0.2on ubuntu (wsl):
christian@chrisPC:~/development/yo$ echo $PNPM_HOME
/home/christian/.local/share/pnpm
christian@chrisPC:~/development/yo$ node lib/cli.js --generators
Available Generators:
office
christian@chrisPC:~/development/yo$ pnpm list -g
Legend: production dependency, optional only, dev only
/home/christian/.local/share/pnpm/global/5
dependencies:
generator-office 3.0.2
src/module-lookup.ts
Outdated
| onlyDirectories: true, | ||
| suppressErrors: true, | ||
| absolute: true, | ||
| }).map(modulesPath => resolve(pnpmHome, modulesPath)), |
There was a problem hiding this comment.
Since we are requesting absolute paths, pnpmHome in resolve should not be required.
| }).map(modulesPath => resolve(pnpmHome, modulesPath)), | |
| }).map(modulesPath => resolve(modulesPath)), |
There was a problem hiding this comment.
You are correct. It is not required. I removed pnpmHome from resolve and repeating the above tests we are able to find my pnpm globally installed generators
windows
PS C:\Users\christian\development\yo> node .\lib\cli.js --generators
Available Generators:
office
ubuntu (wsl)
christian@chrisPC:~/development/yo$ node lib/cli.js --generators
Available Generators:
office
Fixes #693
Purpose of this pull request?
What changes did you make?
Switched to using globbySync when pushing all pnpm node_module directories to paths array. The
global/*/node_modulespath was failing this existsSync check as the directory doesn't actually exist and so none of the pnpm installed generators could be found