feat: support pnpm node_modules style #237
Conversation
Codecov Report
@@ Coverage Diff @@
## master #237 +/- ##
=======================================
Coverage 99.74% 99.74%
=======================================
Files 19 19
Lines 1178 1179 +1
Branches 207 207
=======================================
+ Hits 1175 1176 +1
Misses 3 3
Continue to review full report at Codecov.
|
lib/loader/mixin/plugin.js
Outdated
| } else { | ||
| // should find the siblings directory of framework when using pnpm | ||
| frameworkDepsPath = path.dirname(eggPath); | ||
| if (fs.existsSync(frameworkDepsPath)) { |
There was a problem hiding this comment.
感觉这里是一个 break，会不会带来其它问题，因为不强制去 node_modules 目录查了，假如正常的 eggPath 平级有一些同名 js 文件都会被错误引入
|
对各个包管理工具的 lookupdir 测试了下： # pnpm
'node_modules',
'node_modules/.pnpm/yadan@2.0.0/node_modules/yadan/node_modules',
'node_modules/.pnpm/yadan@2.0.0/node_modules',
'node_modules/.pnpm/egg@2.33.1/node_modules/egg/node_modules',
'node_modules/.pnpm/egg@2.33.1/node_modules'
# tnpm
'egg-showcase/node_modules',
'egg-showcase/node_modules/_yadan@2.0.0@yadan/node_modules',
# (因为 tnpm 是扁平化的，yadan 的父目录是根目录 node_modules，所以被 Set 过滤掉了)
'egg-showcase/node_modules/_egg@2.33.1@egg/node_modules'
# npm
'node_modules',
'node_modules/yadan/node_modules',
# (因为 npm 是扁平化的，yadan 的父目录是根目录 node_modules，所以被 Set 过滤掉了)
'node_modules/egg/node_modules' |
| } | ||
|
|
||
| // should find the $cwd/node_modules when test the plugins under npm3 | ||
| lookupDirs.push(path.join(process.cwd(), 'node_modules')); | ||
| lookupDirs.add(path.join(process.cwd(), 'node_modules')); | ||
|
|
||
| for (let dir of lookupDirs) { |
There was a problem hiding this comment.
这个地方，如果把原来的 fs.exists 改为 require.resolve(${name}/package.json, { paths: [...lookupDirs] }); 其实会更合理，使用到 require 自己的寻址机制，就不用在上面 hard code 目录。
潜在的问题：
- 寻址逻辑稍微复杂了一点，但基本上可以无视，后面支持 manifest 后就更没问题了。
- inline plugin 会存在一些问题，现在我们没有强制检测它必须有 package.json，就看这种我们是否算 break
@fengmk2 @hyj1991 @mansonchor @popomore 你们怎么看？
There was a problem hiding this comment.
好，我也倾向于用 resolve， 不过肯定会 break 到一些不规范的用户的，之前没这个规范，但我们也没有强制限制。
我再提个 PR
There was a problem hiding this comment.
在这个 fn 的第一行，就会判断 plugin.path 是否存在，然后直接 return 了，所以不存在上面说的 break 的情况。
重新提交了下 #238
|
@atian25 你来发版本。 |
|
额。。。 这个 PR 还没改为 resolve，我再提一个 PR 吧 |
|
发了 4.22.0， resolve 那个我晚点再提一个 |
Checklist
npm testpassesAffected core subsystem(s)
Description of change
due to pnpm's Symlinked
node_modulesstructurewe should locate plugin not only from the framework's
node_modulesbut also the siblings directory.close eggjs/egg#4739