♻️ move browser-core to devDependencies#78
Conversation
bcaudan
force-pushed
the
bcaudna/fix-dep
branch
from
cc618a0 to
52a4252
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52a42522d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- use `respectExternal: true` in rollup-plugin-dts so only `electron` is treated as external - browser-core types (`DefaultPrivacyLevel`, `Context`) are now inlined in `dist/index.d.ts`
cdn34dd
left a comment
cdn34dd
left a comment
There was a problem hiding this comment.
Nice catch! One thing though, the distinction between dependencies and devDependencies can be subtle when using a bundler like rollup, and it's pretty easy to get wrong because things wiill work fine either way locally.
Could we add a quick note maybe in the CONTRIBUTING.md file highlighting where a new package should be added as ?
2 participants
Motivation
@datadog/browser-corewas listed as a production dependency, but Rollup bundles it entirely into the distribution output. Listing it as a prod dep was inaccurate and would unnecessarily inflate the dependency tree of downstream consumers.Changes
@datadog/browser-corefromdependenciestodevDependenciesTest instructions
yarn build— bundle builds without errorsrequire('@datadog/browser-core')appears indist/outputyarn test:unit— all unit tests passChecklist