-
Notifications
You must be signed in to change notification settings - Fork 9
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
install stack-utils
to all packages that inline the shared folder
#594
Conversation
🦋 Changeset detectedLatest commit: a2e3d56 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
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.
Approve! but with a change suggested
package.json
Outdated
@@ -41,5 +42,8 @@ | |||
"@jest/environment@npm:^27.5.1": "patch:@jest/environment@npm%3A28.1.3#~/.yarn/patches/@jest-environment-npm-28.1.3-506a81a227.patch", | |||
"@jest/environment@npm:^28.1.3": "patch:@jest/environment@npm%3A28.1.3#~/.yarn/patches/@jest-environment-npm-28.1.3-506a81a227.patch", | |||
"superstruct": "patch:superstruct@npm%3A1.0.4#~/.yarn/patches/superstruct-npm-1.0.4-44d328b887.patch" | |||
}, | |||
"dependencies": { | |||
"stack-utils": "^2.0.6" |
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.
I don't think we need to (or should?) add this to the root package.json
. That's a private
package anyway so it won't be published anywhere.
We probably should add it to the one in packages/replay
though (even if we don't plan any more releases of this project)
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.
Okay
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.
I think this looks right. If CI is happy we can test it quickly before publishing.
/release-pr |
Don't forget to add a changeset file to this PR so this change will be picked up by all of the packages you've updated. Once that's done we should be unblocked to merge this, and then merge the "Versions" PR that will be auto-opened. |
@@ -57,6 +58,7 @@ | |||
"p-map": "^4.0.0", | |||
"semver": "^7.5.2", | |||
"sha-1": "^1.0.0", | |||
"stack-utils": "^2.0.6", |
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.
this should catch such issues early:
replay-cli/scripts/pkg-build/src/plugins/resolveErrors.ts
Lines 29 to 35 in 78f5c72
if (!source.startsWith(".") && !source.startsWith("/") && !isExternal(source)) { | |
throw new Error( | |
`"${source}" is imported ${ | |
importer ? `by "${importer}" ` : "" | |
}but the package is not specified in dependencies or peerDependencies` | |
); | |
} |
I wonder why it didn't let us know about it here... I'll investigate tomorrow
This fixes the bug report here: https://discord.com/channels/779097926135054346/798693728999047203/1258136045619384420