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

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

Merged
merged 10 commits into from
Apr 15, 2024
Merged

Conversation

cotsupa
Copy link
Contributor

@cotsupa cotsupa commented Apr 8, 2024

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 [ ])

Copy link

salesforce-cla bot commented Apr 8, 2024

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

@filmaj
Copy link
Contributor

filmaj commented Apr 8, 2024

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
Copy link
Contributor Author

cotsupa commented Apr 9, 2024

@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
Copy link
Contributor

filmaj commented Apr 9, 2024

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
Copy link
Contributor Author

cotsupa commented Apr 9, 2024

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
Copy link
Contributor

filmaj commented Apr 10, 2024

@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
Copy link
Contributor Author

cotsupa commented Apr 10, 2024

@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
Copy link
Contributor

filmaj commented Apr 10, 2024

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
Copy link
Contributor Author

cotsupa commented Apr 11, 2024

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
Copy link
Contributor Author

cotsupa commented Apr 15, 2024

@filmaj Please confirm this commit.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

I left one small tweak suggestion, but LGTM otherwise.

packages/web-api/package.json Show resolved Hide resolved
Co-authored-by: Fil Maj <maj.fil@gmail.com>
@cotsupa
Copy link
Contributor Author

cotsupa commented Apr 15, 2024

@filmaj Thanks for the suggestion. Please check again!

@filmaj
Copy link
Contributor

filmaj commented Apr 15, 2024

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
Copy link
Contributor Author

cotsupa commented Apr 15, 2024

@filmaj Thanks for the comment. Please check again!

@filmaj filmaj added area:typescript issues that specifically impact using the package from typescript projects pkg:web-api applies to `@slack/web-api` labels Apr 15, 2024
@filmaj filmaj merged commit ae9ace8 into slackapi:main Apr 15, 2024
17 checks passed
@filmaj filmaj added this to the web-api@7.0.3 milestone Apr 15, 2024
@cotsupa cotsupa deleted the update-p-retry branch April 15, 2024 17:02
@filmaj
Copy link
Contributor

filmaj commented Apr 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typescript issues that specifically impact using the package from typescript projects cla:signed pkg:web-api applies to `@slack/web-api` semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants