Skip to content
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

ci: test samples and examples with a packaged bolt build #2372

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zimeg
Copy link
Member

@zimeg zimeg commented Dec 17, 2024

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

@zimeg 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 zimeg added this to the 4.2.1 milestone Dec 17, 2024
@zimeg zimeg self-assigned this Dec 17, 2024
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.59%. Comparing base (d07a70a) to head (5ea64ca).

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.

Copy link
Member Author

@zimeg zimeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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...

Comment on lines +25 to +29
- 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating examples continues to be an upcoming task, but I found this app to be helpful for testing the current edges.

FWIW I'm interested in knowing if we want to include jsdoc as part of examples for typechecking like this or not? That can of course be for a follow up, but I think it might be neat 🔍

Comment on lines +3 to +6
/**
* @type {import("@slack/bolt/dist/receivers/ParamsIncomingMessage").ParamsIncomingMessage};
*/
const _example = null;
Copy link
Member Author

@zimeg zimeg Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I think ParamsIncomingMessage should be exported at the top-level of @slack/bolt since it describes the "req" parameter of receivers, but that's unchanged in this PR.

However I'm super curious to know if exporting it from the top-level would fix the current typescript error here 🤔

@zimeg 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 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 zimeg requested review from seratch, WilliamBergamin and a team December 17, 2024 06:25
@zimeg zimeg marked this pull request as ready for review December 17, 2024 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M-T: An issue where more input is needed to reach a decision semver:patch tests M-T: Testing work only TypeScript-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant