Implement JavaScript-based constraints #220
Conversation
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
Unfortunately this causes CI to fail, so it looks like we may want to drop support for Node.js 16 (EOL) anyway. |
Mrtenz
force-pushed
the
mrtenz/js-constraints
branch
from
a0da5b1
to
db52e4f
Compare
| @@ -1,3 +1,7 @@ | |||
| compressionLevel: mixed | |||
There was a problem hiding this comment.
What's the reason for these new config options?
There was a problem hiding this comment.
They were added by Yarn when updating the version. I haven't changed anything.
| }); | ||
| } | ||
|
|
||
| module.exports = defineConfig({ |
There was a problem hiding this comment.
This is the first time I'm looking at the new constraints API up close, and unfortunately it's not as readable or understandable as I'd hoped (I should really submit a pull request to the Yarn repo). That said, wrapping each step in a well-named function is a good idea, and the fact that we can read files and do other kinds of things is a big win.
I just had some comments but overall this is looking good.
yarn.config.cjs
Outdated
|
|
||
| // The list of files included in the package must only include files | ||
| // generated during the build process. | ||
| workspace.set('files[0]', 'dist'); |
There was a problem hiding this comment.
It looks like this says that dist must be the first item in files. Does that matter? Should we say that dist must be present in files instead?
There was a problem hiding this comment.
Order of files doesn't matter. I've changed it to workspace.set('files', ['dist']) to make this clear. We could also check if the array contains dist, but I don't think that's possible with workspace.set.
There was a problem hiding this comment.
I think that's okay. Projects can customize this if they really need to, but I imagine that will be rare.
Mrtenz
force-pushed
the
mrtenz/js-constraints
branch
from
99217a4
to
984dc40
Compare
I've replaced the Prolog-based constraints (deprecated in Yarn v4) with JavaScript-based constraints. This has the benefit that the constraints file is much more readable, especially for those who are not well-versed in Prolog. As an added benefit, the error messages produced by Yarn are more readable too.
The new constraints file implements all the Prolog-based constraints, and some extras. For example, we can now verify that the
README.mddoes not contain the template instructions (if the current repository is not the module template), and that the.nvmrcversion matches with the Node.js version mentioned in theREADME.md. Since the new constraints file has access to all Node.js APIs (includingfs), we have much more flexibility in the kinds of constraints.Using these constraints requires bumping Yarn to v4, and using a minimum Node.js version of 18.12.0. The Node.js version bump is a
.nvmrconly bump, so this shouldn't be a breaking change.Blocked by