feat(npm): Add workspaces support #645
Conversation
Adds support for auto-expanding workspaces to avoid config bloat in monorepos like sentry-javascript
BYK
force-pushed
the
byk/feat/npm-workspaces
branch
from
666d9b0 to
88d3667
Compare
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Just to confirm I understand the intention: We want to avoid that workspaces/monorepos like our JS SDK repo have to register each package as separate publishing targets?
My main question here is: Can we guarantee releasing in the correct dependency order? Right now, we maintain this list primarily to guarantee that we release the packages in correct order. So if publishing @sentry/browser fails, that's okay because didn't yet publish any packages depending on it (@sentry/react).
|
Ah, great yeah I think we can do that. Will update the patch accordingly |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Thanks for adding this! Excited to give it a try.
Had some comments, mostly non-blocking low-prio stuff. Let's just clarify the publishConfig comment. After that, good to go from my end!
| expect(names.indexOf('@sentry/types')).toBeLessThan(names.indexOf('@sentry/core')); | ||
| expect(names.indexOf('@sentry/core')).toBeLessThan(names.indexOf('@sentry/browser')); | ||
| expect(names.indexOf('@sentry/core')).toBeLessThan(names.indexOf('@sentry/node')); | ||
| expect(names.indexOf('@sentry/browser')).toBeLessThan(names.indexOf('@sentry/integrations')); | ||
| expect(names.indexOf('@sentry/node')).toBeLessThan(names.indexOf('@sentry/integrations')); |
There was a problem hiding this comment.
this is a good test and depicts something like @sentry/nextjs really well that also depends on browser and node, thanks for adding!
L: Could we add @sentry/node-core as a dependency of @sentry/node (where node-core also depends on core)? Then we have a second "branch" in the graph.
Super-L: WDYT about checking against the names array directly? I think the indexOf is good enough but unless there's something that makes the ordering non-deterministic, the "equals array" (or inline snapshot, whatever you prefer) check makes it super easy to read the test.
(Only suggesting because this is probably the most crucial part to this change. Feel free to disregard)
There was a problem hiding this comment.
Added the node-core test. Will see for the other request :)
Adds support for auto-expanding workspaces to avoid config bloat in monorepos like sentry-javascript