Closed tommy-mitchell closed 1 year ago
I don't want to minify the code. Debugging is important. It also doesn't save much anyway.
How should dependencies in package.json be specified?
They need to be devDependencies, but it would be nice to somehow indicate which are used in production. We could maybe create a custom field like in package.json: {"_actualDependencies": ["..."]}
.
They need to be devDependencies, but it would be nice to somehow indicate which are used in production. We could maybe create a custom field like in package.json:
{"_actualDependencies": ["..."]}
.
Would bundleDependencies
work?
Would bundleDependencies work?
I don't think so. It would bundle the dependencies. So we would still have double up of the code (both rolled-up and npm bundled).
Fair enough. I'll add a custom field. If we're not minimizing, maybe we should only bundle the dependencies?
\__ dependencies/
\__ licenses.md
\__ bundle.js
\__ source/
\__ index.js
\__ ...
\__ index.d.ts
\__ license
\__ package.json
\__ readme.md
Avoids the abbreviation problem too, lol
I think it's easier to simply bundle everything. If we do a separate dependencies file, we need to change all the import statements.
Whoops, you're right, was thinking a little too fast there.
I'm not sure when the build script should be run. Pros and cons:
prepack
:
yarn
git
np
does checks (e.g. npm pack --dry-run --json
)
prepublishOnly
:
npm publish
git
consumers can't consume meow
@tommy-mitchell FYI, Got
and Ky
are using prepare
.
Either prepack
or prepare
. I generally prefer prepare
as it also executes when running npm install
while developing.
Changes:
_actualDependencies
field to package.json
build
instead of dist
prepare
instead of prepack
With Rollup, I was able to separate the dependencies into a separate file and have the imports be updated:
\__ build/
\__ licenses.md # License of each bundled dependency
\__ dependencies.js # Vendor bundle
\__ index.js
\__ options.js
\__ parser.js
\__ utils.js
\__ validate.js
\__ index.d.ts
\__ license
\__ package.json
\__ readme.md
I guess it doesn't hurt, but kinda weird that each generated file includes:
import 'util';
import 'path';
import 'fs';
import 'url';
import 'node:fs';
import 'os';
import 'node:path';
import 'node:url';
import 'node:process';
Looks pretty good
Have you manually tested it with a CLI to make sure meow
still works when published? You can run npm pack
and then decompress it in a CLI project.
Have you manually tested it
I had before, but I went ahead and added a couple of tests using the built output to verify going forward.
I guess it doesn't hurt, but kinda weird that each generated file includes
Yeah I'm not sure what's up with that, I need to do some research.
Managed to fix the empty Node builtin imports. I think this should be good to go.
@sindresorhus @tommy-mitchell One question about this PR: How does it work with security checks and resolution bumps? Since dependencies are now bundled and not declared in the package.json, security issues with them might not be reported anymore by bots... we will also not be able to properly check if a dependency is updated
In the old version:
β project git:(main) β yarn why semver
ββ normalize-package-data@npm:2.5.0
ββ semver@npm:5.7.2 (via npm:2 || 3 || 4 || 5)
β project git:(main) β yarn why normalize-package-data
ββ meow@npm:6.1.1
β ββ normalize-package-data@npm:2.5.0 (via npm:^2.5.0)
β
ββ read-pkg@npm:5.2.0
ββ normalize-package-data@npm:2.5.0 (via npm:^2.5.0)
In the new bundled deps version (12.1.1):
β project git:(main) β yarn why normalize-package-data
ββ read-pkg@npm:5.2.0
ββ normalize-package-data@npm:2.5.0 (via npm:^2.5.0)
No report, although normalize-package-data
it's being used by meow
Also, how does security issues are auto checked in this repo?
not be reported anymore by bots
I would argue that's an upside. 99% of all npm audits are not actually vulnerabilities (the semver
"vulnerability" is a good example of one) or not applicative to meow
. So this reduces the noise.
@sindresorhus I agree that audit sucks. Big red "critical" text scares security company monke
But as Dan says "Inlining dependencies kind of goes against the whole point of npm", and I feel like we kinda lose any analysis (you might not only use it for security) tools using that approach:
But in the end it's your decision & opinion as a lib owner, as long as you're aware I think its ok
I generally agree that inlining is bad, but in this specific case, the upsides outweight the downsides.
I'm on the fence with this change⦠but also I think it would be great to see it on ora
, which seems to be super small but in reality:
At the end of the day bundling is great for this sort of dependencies, but it does force you to release more often than you normally would. This happens for example when a new node version is out and some old issue now needs to be fixed here as well (with a simple sub-dependency patch update)
Closes #67. Bumps dev dependencies and adds an
_actualDependencies
field.Builds on
prepare
and outputs a bundleddependencies.js
andlicenses.md
to thebuild
folder: