ci: test samples and examples with a packaged bolt build by zimeg · Pull Request #2372 · slackapi/bolt-js

zimeg · 2024-12-17T05:07:28Z

Summary

This PR explores broken builds due to missing @types/express from the most recent release 👀

> tsc --noemit --module commonjs --project ./tsconfig.json
node_modules/@slack/bolt/dist/receivers/ExpressReceiver.d.ts(9,98): error TS7016: Could not find a declaration file for module 'express'. '/home/runner/work/slack-sandbox/slack-sandbox/js.bolt.tails/node_modules/express/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/express` if it exists or add a new declaration (.d.ts) file containing `declare module 'express';`
node_modules/@slack/bolt/dist/receivers/ParamsIncomingMessage.d.ts(3,39): error TS2307: Cannot find module 'express-serve-static-core' or its corresponding type declarations.

Preview

This test passes when using npm pack without removing node_modules
This test fails when using npm pack and removing node_modules

Notes

Moving @types/express back into dependencies seems like a fix to the above errors, but this was difficult to confirm with local setups. Hoping to improve our CI testing and return @types/express in perhaps another PR to prevent this regression in future changes 😓
The PR isn't in a state to be merged right now, but I'm super interested in feedback on this testing approach!

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2024-12-17T05:08:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.59%. Comparing base (dcfbab3) to head (d72132f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2372   +/-   ##
=======================================
  Coverage   92.59%   92.59%           
=======================================
  Files          36       36           
  Lines        7472     7472           
  Branches      653      653           
=======================================
  Hits         6919     6919           
  Misses        545      545           
  Partials        8        8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zimeg

📝 Leaving a few notes from findings during the most recent adventures. I'm so open to thoughts and changing the goal of this PR.

IMO testing samples with npm pack might be nice to split into a separate change, but I'm rambling now...

zimeg · 2024-12-17T06:14:51Z

.github/workflows/samples.yml

+      - name: Package the latest changes to bolt-js
+        run: npm i && npm pack . && rm -rf node_modules
+      - name: Install example dependencies for testing
        working-directory: ${{ matrix.example }}
-        run: npm i && npm link @slack/bolt
+        run: npm i && npm i ../../slack-bolt-*.tgz


Some TILs are included here, though I believe this can still be improved:

npm pack will package @slack/bolt as if it's going to be released

node_modules of the bolt-js repo must be removed or nested directories can make use of these pacakges

The *.tgz file seems ideal for testing in a production environment like this since it lets us build bolt-js then remove its node_modules before testing a sample!

I'm not thrilled with the relative paths here, but am interested in if this is an approach we're alright taking?

Using relative path here should be fine, if something changes and this path is broken, the test should fail and inform us that the path is broken

examples/oauth-express-receiver/package.json

examples/oauth-express-receiver/app.js

zimeg

📝 A few notes for the kind reviewer!

With the changes in #2421 landing, tests across these samples are passing. I plan to follow up with similar skipLibCheck updates across sample apps used in testing, but for now I think this contains a nice improvement to tests 🔬 ✨

zimeg · 2025-02-10T22:34:34Z

examples/custom-receiver/tsconfig.json

    "resolveJsonModule": true,
    "rootDir": "src",
-    "skipLibCheck": true,
+    "skipLibCheck": false,


When this is set to true, build errors relating to other packages do not appear. Reference 📚

That includes what was surfaced in #2409 👾

Nice thanks 💯 it makes sense to set this as false, allows the CI to validate against the packages used by the project

zimeg · 2025-02-10T22:35:17Z

examples/getting-started-typescript/tsconfig.json

    "resolveJsonModule": true,
    "allowSyntheticDefaultImports": true,
    "allowJs": true,
+    "skipLibCheck": false,


This overrides the default extend value: https://www.npmjs.com/package/@tsconfig/node18

WilliamBergamin

This is great 💯 using npm pack rather then npm link brings this test closer to the end user experience and should allow us to catch unexpected behaviors

thanks for working on this 🚀

Any thoughts on updating the maintainers_guide to recommend npm pack for local testing?

WilliamBergamin · 2025-02-11T15:34:37Z

examples/custom-receiver/tsconfig.json

    "resolveJsonModule": true,
    "rootDir": "src",
-    "skipLibCheck": true,
+    "skipLibCheck": false,


Nice thanks 💯 it makes sense to set this as false, allows the CI to validate against the packages used by the project

WilliamBergamin · 2025-02-11T15:39:08Z

.github/workflows/samples.yml

+      - name: Package the latest changes to bolt-js
+        run: npm i && npm pack . && rm -rf node_modules
+      - name: Install example dependencies for testing
        working-directory: ${{ matrix.example }}
-        run: npm i && npm link @slack/bolt
+        run: npm i && npm i ../../slack-bolt-*.tgz


Using relative path here should be fine, if something changes and this path is broken, the test should fail and inform us that the path is broken

zimeg · 2025-02-11T17:55:49Z

@WilliamBergamin thanks a ton for the review and feedback! I'm hoping to merge this soon, but am sharing a few more thoughts:

using npm pack rather then npm link brings this test closer to the end user experience and should allow us to catch unexpected behaviors

I'm most excited for this. Finding this gap in our testing was difficult with a combination of the above tsconfig example settings and running tests with devDependences installed. I'm hoping we don't find surprises in these packages 🎁 ✨

Any thoughts on updating the maintainers_guide to recommend npm pack for local testing?

Super great call and added in d72132f! I was worried this would add some overhead to fast iteration but I remembered the npm run build script is required to use the latest changes anyways, so replacing this with npm pack isn't too bad!

The tests are all passing now, but before merging I want to make a few changes to the typescript samples found on @slack-samples to match the included examples, then re-run the tests. Hoping nothing changes with this, but I can't be certain 🚢

zimeg · 2025-02-11T18:12:52Z

☝️ Changes to samples have been made, but are failing tests with the current @slack/bolt release (IMO this is "good"!) so I'll hold off on merging those for now.

With the other updates made here I think it's a good time to merge this. All for the sake of testing 🔬 ✨

WilliamBergamin

Nice thanks for making this happen 💯

I left 2 comments one is a NIT that can be ignored if you feel the style is off

WilliamBergamin · 2025-02-11T18:20:33Z

.github/maintainers_guide.md


+#### Local Development
+
+Using in progress changes made to this package in another app is sometimes useful in experiments. A packaged build of Bolt for JavaScript can be created with `npm pack` and installed in a project with `npm install ../slack-bolt-*.tgz`. The packaged build includes dependencies published with Bolt for JavaScript, including required peer dependencies but not devDependencies, to imitate actual installations.


Nit: what do you think about making this in point/step format

Suggested change

Using in progress changes made to this package in another app is sometimes useful in experiments. A packaged build of Bolt for JavaScript can be created with `npm pack` and installed in a project with `npm install ../slack-bolt-*.tgz`. The packaged build includes dependencies published with Bolt for JavaScript, including required peer dependencies but not devDependencies, to imitate actual installations.

Using in progress changes made to this package in an app can be useful for development.

Use the pack command to package the project

```zsh

npm pack

```

Install the `slack-bolt-*.tgz` to use your changes

```zsh

npm install path/to/bolt-js/slack-bolt-*.tgz

```

The packaged build includes dependencies published with Bolt for JavaScript, including required peer dependencies but not devDependencies, to imitate actual installations.

WilliamBergamin · 2025-02-11T18:24:22Z

.github/maintainers_guide.md

+
+Using in progress changes made to this package in another app is sometimes useful in experiments. A packaged build of Bolt for JavaScript can be created with `npm pack` and installed in a project with `npm install ../slack-bolt-*.tgz`. The packaged build includes dependencies published with Bolt for JavaScript, including required peer dependencies but not devDependencies, to imitate actual installations.
+
+Remove cached project dependencies with `rm -rf node_modules` between those steps to keep the cache clean.


Maybe we should mention removing the package-lock.json as well 🤔

And I don't think we need the force flag for this, but correct me if I'm wrong

Suggested change

Remove cached project dependencies with `rm -rf node_modules` between those steps to keep the cache clean.

Remove cached project dependencies with `rm -r node_modules package-lock.json ` between those steps to keep the cache clean.

I've been relying on the npm command to keep the package-lock.json most relevant, but for development it seems alright to regenerate this afresh! Good call 👏

I also tend to prefer rm -rf because it's "really fast" but sometimes permissions catch me 😉

zimeg · 2025-02-11T18:37:18Z

@WilliamBergamin Ooh I'm way more a fan of spaced sections. I was hoping to match the surrounding sections with this change, but IMO revisiting this guide to use command sections would be most ideal 👁️‍🗨️

ci: attempt to test typescript samples without dev deps

3fba83d

zimeg added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch TypeScript-specific dependencies Pull requests that update a dependency file labels Dec 17, 2024

zimeg added this to the 4.2.1 milestone Dec 17, 2024

zimeg self-assigned this Dec 17, 2024

zimeg added 5 commits December 16, 2024 21:29

ci: package the changes instead of linking a checkout

3d8f5a3

fix: jump two directories up to the packaged tgz

ab2504f

ci: perform typechecking on the oauth express receiver js app

e5ad367

test: add a type statement that requires unmet dependencies

155d38b

ci: remove bolt-js dependencies before testing apps

5ea64ca

zimeg commented Dec 17, 2024

View reviewed changes

zimeg changed the title ~~build: revert installing @types/express under the dev dependencies~~ ci: test samples and examples with a packaged bolt build Dec 17, 2024

zimeg added discussion M-T: An issue where more input is needed to reach a decision tests M-T: Testing work only and removed bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented dependencies Pull requests that update a dependency file labels Dec 17, 2024

zimeg requested review from a team, WilliamBergamin and seratch December 17, 2024 06:25

zimeg marked this pull request as ready for review December 17, 2024 06:25

zimeg marked this pull request as draft January 9, 2025 01:00

zimeg modified the milestones: 4.2.1, 4.x Feb 10, 2025

zimeg added 4 commits February 10, 2025 14:14

chore: merge w main

0cb53c8

revert: undo changes that tested an edge case locally

994f2c6

ci: remove builds of the javascript oauth express example

cd042e1

fix: avoid skipping lib checks when building typescript examples

b84b62c

zimeg commented Feb 10, 2025

View reviewed changes

zimeg marked this pull request as ready for review February 10, 2025 22:37

WilliamBergamin approved these changes Feb 11, 2025

View reviewed changes

docs: write steps to develop with 'npm pack' for maintainers

d72132f

This was referenced Feb 11, 2025

build: include dependency types when checking types slack-samples/bolt-ts-starter-template#141

Merged

build: include dependency types when checking types slack-samples/bolt-ts-custom-step-template#22

Merged

zimeg merged commit 5b5bd89 into main Feb 11, 2025
18 checks passed

zimeg deleted the zimeg-fix-express-types-dependency branch February 11, 2025 18:13

WilliamBergamin requested changes Feb 11, 2025

View reviewed changes

WilliamBergamin mentioned this pull request Feb 11, 2025

fix: improve the maintainers_guide local development instruction #2422

Merged

2 tasks


		#### Local Development

		Using in progress changes made to this package in another app is sometimes useful in experiments. A packaged build of Bolt for JavaScript can be created with `npm pack` and installed in a project with `npm install ../slack-bolt-*.tgz`. The packaged build includes dependencies published with Bolt for JavaScript, including required peer dependencies but not devDependencies, to imitate actual installations.

-Using in progress changes made to this package in another app is sometimes useful in experiments. A packaged build of Bolt for JavaScript can be created with `npm pack` and installed in a project with `npm install ../slack-bolt-*.tgz`. The packaged build includes dependencies published with Bolt for JavaScript, including required peer dependencies but not devDependencies, to imitate actual installations.
+Using in progress changes made to this package in an app can be useful for development.
+ Use the pack command to package the project
+ ```zsh
+ npm pack
+ ```
+Install the `slack-bolt-*.tgz` to use your changes
+```zsh
+npm install path/to/bolt-js/slack-bolt-*.tgz
+```
+The packaged build includes dependencies published with Bolt for JavaScript, including required peer dependencies but not devDependencies, to imitate actual installations.


		Using in progress changes made to this package in another app is sometimes useful in experiments. A packaged build of Bolt for JavaScript can be created with `npm pack` and installed in a project with `npm install ../slack-bolt-*.tgz`. The packaged build includes dependencies published with Bolt for JavaScript, including required peer dependencies but not devDependencies, to imitate actual installations.

		Remove cached project dependencies with `rm -rf node_modules` between those steps to keep the cache clean.

Comments

Conversation

zimeg commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Preview

Notes

Requirements

Uh oh!

codecov bot commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilliamBergamin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg commented Feb 11, 2025

Uh oh!

zimeg commented Feb 11, 2025

Uh oh!

Uh oh!

WilliamBergamin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg commented Feb 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zimeg commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading