-
Notifications
You must be signed in to change notification settings - Fork 678
build: setup monorepo npm workspace for dependent packages #2478
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2478 +/- ##
==========================================
- Coverage 93.09% 93.09% -0.01%
==========================================
Files 40 40
Lines 11240 11239 -1
Branches 713 713
==========================================
- Hits 10464 10463 -1
Misses 764 764
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
zimeg
left a comment
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.
📬 A few messages for the kind reviewers!
| - name: Build packages | ||
| run: | | ||
| # Build packages without internal dependencies | ||
| npm run build --workspace=@slack/cli-hooks | ||
| npm run build --workspace=@slack/cli-test | ||
|
|
||
| # Build base dependencies | ||
| npm run build --workspace=@slack/logger | ||
| npm run build --workspace=@slack/types | ||
|
|
||
| # Build packages requiring base dependencies | ||
| npm run build --workspace=@slack/web-api | ||
| npm run build --workspace=@slack/webhook | ||
|
|
||
| # Build packages that depend on the Web API | ||
| npm run build --workspace=@slack/oauth | ||
| npm run build --workspace=@slack/rtm-api | ||
| npm run build --workspace=@slack/socket-mode |
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.
🗣️ note: #2482 is an example of changing dependencies between packages being resolved as expected!
In that example, changes to @slack/types are required in @slack/web-api which cause tests on matching commit f2ac218 to fail in an unchanged branch. We might hope to include these changes as part of related PRs for confident CI without updates across prereleases 🫡
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 is also so much easier to read and maintain!
|
|
||
| The script places the reference markdown files in `/docs/english/reference/package-name`. | ||
|
|
||
| ### 🚀 Releases |
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.
🔮 note: These steps are verbose now but I'm hoping to fast follow this change with some enhancement to the release process! For now these steps still package as expected, testing with the following:
$ npm pack --workspace=@slack/web-api --dry-run
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.
📠 note: Apologies for noise in these changes, but a lot of formatting happened I fear.
| "url": "https://github.com/slackapi/node-slack-sdk/issues" | ||
| }, | ||
| "scripts": { | ||
| "prepare": "npm run build", |
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.
🗣️ note: The prepare script is replaced with prepack in all packages!
This helps prevent installation and build errors between packages on an initial install. Otherwise, packages are built when dependencies are installed with the prepare script, and the oauth package requires web-api is built but these are packaged in alphabetical order. Overrides for this aren't obvious to me...
AFAICT this isn't a breaking change since the prepack script is run still when packages are installed from Git. FWIW I cannot figure out how to install packages in a monorepo with that method in both cases either 🤖
| "scripts": { | ||
| "lint": "npx @biomejs/biome check packages", | ||
| "lint:fix": "npx @biomejs/biome check --write packages" | ||
| }, |
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.
🪵 note: Linting becomes a global task with impressive speeds of biome - less than 1 second for all packages.
This is a noticed change to the maintenance tasks I think, but --workspace commands are preferred for package scripts overall, which are run from the root:
$ npm run test --workspace=@slack/webhook
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.
🌲 note: I'm so open to adding more scripts here too, but hope to keep this PR scoped to minimal improvements of workspaces to start:
- Automatic dependent and deduplicated package resolution in development
- Shared linting scripts
- Test fixes and simplified linking related to dependencies
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.
Let's keep this PR minimal and scoped, but we can always add more to scripts in future work!
I think a root-level npm test script would be nice to match our npm run lint. Future work!
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.
@mwbrooks Nice callout! A shared test script sounds so amazing for testing changes that span packages 🧪 ✨
srtaalej
left a comment
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.
looking forward to using this ⭐ 🤩
mwbrooks
left a comment
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.
✅ Praise goes to @zimeg and this pull request!
- Significantly simplifies the CI workflow
- Centralized linting and reduced biome duplication
- Dependabot tracks the root-level npm dependency
| - name: Build packages | ||
| run: | | ||
| # Build packages without internal dependencies | ||
| npm run build --workspace=@slack/cli-hooks | ||
| npm run build --workspace=@slack/cli-test | ||
|
|
||
| # Build base dependencies | ||
| npm run build --workspace=@slack/logger | ||
| npm run build --workspace=@slack/types | ||
|
|
||
| # Build packages requiring base dependencies | ||
| npm run build --workspace=@slack/web-api | ||
| npm run build --workspace=@slack/webhook | ||
|
|
||
| # Build packages that depend on the Web API | ||
| npm run build --workspace=@slack/oauth | ||
| npm run build --workspace=@slack/rtm-api | ||
| npm run build --workspace=@slack/socket-mode |
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 is also so much easier to read and maintain!
|
|
||
| Labels are used to run issues through an organized workflow. Here are the basic definitions: | ||
|
|
||
| * `bug`: A confirmed bug report. A bug is considered confirmed when reproduction steps have been documented and the |
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.
praise: Nice find!
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.
@mwbrooks Thanks for taking notice! The leading * can be confusing with later bold formatting and the linter I use replaces this kind!
| "useIgnoreFile": true | ||
| } | ||
| }, | ||
| "overrides": [ |
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.
❤️
| "scripts": { | ||
| "lint": "npx @biomejs/biome check packages", | ||
| "lint:fix": "npx @biomejs/biome check --write packages" | ||
| }, |
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.
Let's keep this PR minimal and scoped, but we can always add more to scripts in future work!
I think a root-level npm test script would be nice to match our npm run lint. Future work!
|
|
||
| ```sh | ||
| npm install path/to/node-slack-sdk/packages/slack-web-api-*.tgz | ||
| npm install path/to/node-slack-sdk/slack-web-api-*.tgz |
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.
question: Is this path correct? Or, should it include /packages/?
| npm install path/to/node-slack-sdk/slack-web-api-*.tgz | |
| npm install path/to/node-slack-sdk/packages/slack-web-api-*.tgz |
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.
@mwbrooks I fear the earlier change was missing the actual package path itself! But now in a monorepo setup these packages are built at the top level 🎁
zimeg
left a comment
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.
🔭 A final note on documentation before merging soon!
| npm run docs | ||
| npm run docs --workspace packages/web-api |
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.
📚 note: The existence of workspaces adds more information to reference that I think is meaningful to keep, but I want to call this out as a change:
- Defined in: [src/errors.ts:13](https://github.com/slackapi/node-slack-sdk/blob/main/packages/webhook/src/errors.ts#L13)
+ Defined in: [packages/webhook/src/errors.ts:13](https://github.com/slackapi/node-slack-sdk/blob/main/packages/webhook/src/errors.ts#L13)|
@srtaalej @mwbrooks Huge thanks in helping bring these changes to the finish line 🏁 At least for now! We might find more improvements toward maintenance in #2483 but let's merge this now for adjacent tests of #2484! This might also be noted as effort toward #2359 as the latest features of |
Summary
This PR sets up
workspacesfor improved package management.Requirements