refactor: replace read-pkg-up with escalade #187
Conversation
lib/plugins/ramdisk.js
Outdated
| const rm = require('rimraf'); | ||
| const { WebpackPluginRamdisk } = require('webpack-plugin-ramdisk'); | ||
|
|
||
| const { PluginExistsError, RamdiskPathError } = require('../errors'); | ||
|
|
||
| function readPkgName() { |
There was a problem hiding this comment.
| function readPkgName() { | |
| const readPkgName = () => { |
We should export this so it can be tested
|
Heh, I just found currently pkg.name is always null so we always get ramdom directory name. pkg.name is incorrect. Should've been pkg.packageJson.name. And tests are wrong as well. |
|
Current tests covers logic around escalade. Though the code with generating id cannot be reached. Not sure how to cover it with tests as well. |
lib/plugins/ramdisk.js
Outdated
| const readPkgName = () => { | ||
| const pkgPath = escalade(process.cwd(), (dir, names) => { | ||
| if (names.includes('package.json')) { | ||
| return 'package.json'; | ||
| } | ||
| return false; | ||
| }); | ||
| if (pkgPath == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')); | ||
| return pkg.name; | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
we're going to need test coverage of this method before we can merge
There was a problem hiding this comment.
As I mentioned above. This is covered by existing tests. See https://github.com/shellscape/webpack-plugin-serve/pull/187/files#diff-817a4e640cdcf9964df2158c8020f627R33-R54
Though I can add test with broken package.json. This way random name can be tested too.
There was a problem hiding this comment.
No, broken package.json will not work because webpack-nano failed as well for some reason.
|
Looks good. Should we merge (squash please)? |
Ref https://packagephobia.com/result?p=read-pkg-up read-pkg-up is quite big package with many dependencies. It solves cases not necessary for this project like pretty json errors and normalisation of package.json data. In this diff I replaced it with dependency-free escalade and parsed json with native JSON.parse. This will let users to have faster installation and less bloated node_modules.
|
@bebraw Done |
|
Ok, merged. We can even squash on merge without any extra work (one of the options) so maybe we'll use that in the future. 👍 |
|
I enforced only squash commits at my work. Can be configured in settings. |
That's a good idea. @shellscape Do you want to set that up? |
Ref https://packagephobia.com/result?p=read-pkg-up
read-pkg-up is quite big package with many dependencies.
It solves cases not necessary for this project like pretty json errors
and normalisation of package.json data.
In this diff I replaced it with dependency-free escalade and parsed json
with native JSON.parse.
This will let users to have faster installation and less bloated
node_modules.
This PR contains:
Breaking Changes?