-
Notifications
You must be signed in to change notification settings - Fork 253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(shared): Add missing entries to "files" array in package.json #4172
Conversation
🦋 Changeset detectedLatest commit: 85124a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
||
if (pkgFile.files.length !== allFilesNames.length) { | ||
throw new Error('The package.json "files" array length does not match the subpaths.mjs'); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ is this almost the same error as the one that exists right below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .every
error won't fire if the length is not the same, so early return here and make the error clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could merge these 2 checks and throw a single error if any of these checks fail.
@@ -1,5 +1,5 @@ | |||
// This file is a helper for the "subpath-workaround.mjs" script | |||
// We have to polyfill our "exports" subpaths :cry: | |||
// When adding an entry to "subpathNames" also add it to "files" in package.json | |||
|
|||
export const subpathNames = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just replace this with package.files ? In order to use package.json as the source of truth ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The files
array contains directories (like dist
, scripts
, react
) that need special handling and/or shouldn't get subdirectories created. So one way or another you'd need to add special handling. I can adjust the logic in another PR probably so that files
array is the source of truth
Description
The last couple of PRs merged did miss adding the new subpaths to the
files
array in package.json. The script that should have checked it miss it, it'll catch it now.Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change