-
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
Rework the dts generation #576
Conversation
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@rollup/[email protected], npm/[email protected], npm/[email protected] |
@@ -37,6 +37,7 @@ | |||
"@jest/test-result": "^27.5.1", | |||
"@jest/types": "^27.5.1", | |||
"@replay-cli/pkg-build": "workspace:^", | |||
"@replay-cli/shared": "workspace:^", |
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 get a confusing error if a transitive bundled dependency isn't declared here. While we need this situation to error for regular external dependencies, I think we could compute potential bundled dependencies recursively here:
replay-cli/scripts/pkg-build/src/bin.ts
Lines 29 to 33 in 229636e
const isBundledDependency = makePackagePredicate( | |
Object.keys(pkg.packageJson.devDependencies || {}).filter( | |
name => packagesByName.has(name) && !isExternal(name) | |
) | |
); |
I imagine that this recursive search should include dependencies (unlike the top-level one above that looks at devDependencies
)
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.
Stamping to unblock but didn't really read this.
setUserAgent(`${pkgJson.name}/${pkgJson.version}`); | ||
initLogger(pkgJson.name, pkgJson.version); |
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 we should consolidate this into a single call and maybe even consider calling those at the top-level. We don't need to wait for the constructor's call here.
I don't intend to touch this as part of this PR though.
fixes https://linear.app/replay/issue/PRO-740