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

Remove some @ts-ignore #2167

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

QuentinLemCode
Copy link
Contributor

Hello

I have open this PR in order to remove some @ts-ignore comments to improve Typescript type safety.

Types have been adjusted to reflect the correct usage in the codebase.
Some utility function has been added to parse headers to a proper value when needed.

Feel free to ask question and add review comments, I will update as needed

Thanks !

@QuentinLemCode QuentinLemCode requested a review from a team as a code owner September 4, 2024 13:13
@QuentinLemCode QuentinLemCode requested review from helenye-stripe and removed request for a team September 4, 2024 13:13
@xavdid-stripe xavdid-stripe removed the request for review from helenye-stripe September 5, 2024 00:42
@xavdid-stripe
Copy link
Member

Looks like tests didn't run, i'm going to close and re-open the PR to see if it refreshes

@xavdid-stripe
Copy link
Member

@QuentinLemCode looks like the build step is failing with a new type error. Can you address?

@QuentinLemCode
Copy link
Contributor Author

@QuentinLemCode looks like the build step is failing with a new type error. Can you address?

Yes I see, this is caused by the old version of Typescript you use (4.9.4, latest is 5.5). My IDE was using latest version and so it did not show me the error.

I will fix this but I recommend to upgrade your Typescript version which handle type inference more precisely

@prathmesh-stripe
Copy link
Contributor

There is a test failing on Node 12 related to one the changes you made in utils. Can you please fix that?

@QuentinLemCode
Copy link
Contributor Author

There is a test failing on Node 12 related to one the changes you made in utils. Can you please fix that?

Hello @prathmesh-stripe

I found the culprit : the URL interpolator function should work with number, string or boolean. I have added a type guard to ensure the variable as the proper type, otherwize return ''.

This allowed me to spot a potential bug, when using URL interpolator with the number 0, it would make the condition falsy and return an empty string ''
I have added a test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants