slackapi / node-slack-sdk

Slack Developer Kit for Node.js
https://slack.dev/node-slack-sdk
MIT License
3.27k stars 662 forks source link

web-api(fix): Update p-retry package #1772

Closed cotsupa closed 5 months ago

cotsupa commented 5 months ago

Summary

When using web-api, @types/retry may conflict and build may not be possible, so I updated the p-retry package to v6.

node_modules/@slack/web-api/dist/retry-policies.d.ts:5:39 - error TS2312: An interface can only extend an object type or intersection of object types with statically known members.

5 export interface RetryOptions extends OperationOptions {
                                        ~~~~~~~~~~~~~~~~

Found 1 error in node_modules/@slack/web-api/dist/retry-policies.d.ts:5

Requirements (place an x in each [ ])

salesforce-cla[bot] commented 5 months ago

Thanks for the contribution! Before we can merge this, we need @cotsupa to sign the Salesforce Inc. Contributor License Agreement.

filmaj commented 5 months ago

Thanks for the PR! I as well have tried to update this package, but in v6 (and many of sindresorhus' packages), he has moved to an ESM-only approach. You can see in the GitHub Actions run that this fails the test. Not sure if there is a way to be able to do this - perhaps a build configuration tweak would resolve this? Not sure, but if you can figure out a fix for this I would be very appreciative!

cotsupa commented 5 months ago

@filmaj Thank you for your comment! When I looked into p-retry, it turned out to be a pure ESM, and it was recommended to use ESM. It's difficult to convert the web-api to ESM. https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

Just by changing the interface to type, it seems like the following error can be resolved. How about we just make changes to the interface without updating p-retry?

node_modules/@slack/web-api/dist/retry-policies.d.ts:5:39 - error TS2312: An interface can only extend an object type or intersection of object types with statically known members.

5 export interface RetryOptions extends OperationOptions {
                                        ~~~~~~~~~~~~~~~~

Found 1 error in node_modules/@slack/web-api/dist/retry-policies.d.ts:5
filmaj commented 5 months ago

I'm actually not fully understanding how the dependencies work in this case. It seems like we are using this @types repo implicitly, even though it is not in the package.json of this project? https://www.npmjs.com/package/@types/retry

Feel free to try whatever changes you think would work and pass the tests; I am curious to see how one could resolve the issues. I recall spending an hour or two trying to fix this stuff a few months ago and failed to figure it out.

cotsupa commented 5 months ago

This issue may be resolved without upgrading p-retry. I'd like you to merge this commit. I think it'd be better to solve the upgrade of p-retry separately from this issue.

filmaj commented 5 months ago

@cotsupa can you explain where the OperationOptions type is coming from? I would like to understand the import graph better. Is my suspicion that somehow our IDEs are pulling in the @types/retry repo in implicitly correct?

cotsupa commented 5 months ago

@filmaj It reproduces when the local-package dependencies are as follows:

$ npm ls @types/retry
@local-package@1.0.0 /Path/local-package
├─┬ @hoge-package@1.0.0
│ └─┬ @types/async-retry@1.4.8
│   └── @types/retry@0.12.5
└─┬ @slack/web-api@7.0.2
  └─┬ p-retry@4.6.2
    └── @types/retry@0.12.0

In @types/retry version 0.12.5, OperationOptions is defined as follows:

export type OperationOptions = WrapOptions | number[];

Accepting this commit or writing "@types/retry": "0.12.0" in your @slack/web-api dependencies may resolve this issue. I think it's better to accept this commit.

filmaj commented 5 months ago

Yes I would feel better about explicitly depending upon the relevant types package instead of assuming it would come in via a transient dependency. Another concern I have is that the latest @types/retry package is v0.12 while the package with the ACTUAL retry policies is v0.13. We would have to assume the types provided by the @types/retry still match the implied types used by the retry package.

From the library perspective it comes in via the p-retry package for the currently-used version (4.x):

11:22:54 in node-slack-sdk/packages/web-api
➜ npm ls @types/retry
@slack/web-api@7.0.2 /Users/fmaj/src/node-slack-sdk/packages/web-api
└─┬ p-retry@4.6.2
  └── @types/retry@0.12.0
cotsupa commented 5 months ago

Thank you for your acceptance. I have committed the fix to add it to the dependencies, so please check it. Indeed, there are concerns, but since we are dependent on p-retry, I think it's fine as it is. The test passes locally, so I would like you to try again.

cotsupa commented 5 months ago

@filmaj Please confirm this commit.

cotsupa commented 5 months ago

@filmaj Thanks for the suggestion. Please check again!

filmaj commented 5 months ago

Lol I see why you locked to a specific version of the types now 🤦 if you can revert that last commit I swear we will get this merge. My apologies

cotsupa commented 5 months ago

@filmaj Thanks for the comment. Please check again!

filmaj commented 5 months ago

Released on npm as 7.0.3! Thanks so much. Full release is here: https://github.com/slackapi/node-slack-sdk/releases/tag/%40slack%2Fweb-api%407.0.3