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

[SDK-3870] Typescript Migration #628

Merged
merged 39 commits into from
Jun 5, 2023
Merged

[SDK-3870] Typescript Migration #628

merged 39 commits into from
Jun 5, 2023

Conversation

poovamraj
Copy link
Contributor

@poovamraj poovamraj commented May 25, 2023

Changes

We are migrating to Typescript to provide better developer experience. Things to consider in this PR are

  • We have migrated all our source code to strict typescript migration (except tests and mocks)
  • The folder webauth can be reviewed a little leniently as we will move it to native code in the current initiative

Testing

  • This change adds unit test coverage
  • This change has been tested on the latest version of the platform/language or why not

@poovamraj poovamraj requested a review from a team as a code owner May 25, 2023 14:06
@poovamraj poovamraj marked this pull request as draft May 25, 2023 14:08
@poovamraj poovamraj marked this pull request as ready for review May 30, 2023 06:35
Copy link
Member

@frederikprijck frederikprijck left a comment

Choose a reason for hiding this comment

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

Nice work on migrating this to TypeScript 🚀

I left a couple of comments, but in general I think it's useful to revisit the amount of any's being used. There are situations where I think it's okay to keep any, but quite a bit (especially when it comes to option parameters) should probably better be typed.

Some any's could also be unknown, which in that case might something we want to consider using (I added a comment for an example on where I think it should be possible to use unknown).

src/hooks/auth0-provider.tsx Outdated Show resolved Hide resolved
src/hooks/auth0-provider.tsx Outdated Show resolved Hide resolved
src/hooks/auth0-provider.tsx Outdated Show resolved Hide resolved
src/hooks/auth0-provider.tsx Outdated Show resolved Hide resolved
src/hooks/auth0-provider.tsx Outdated Show resolved Hide resolved
src/hooks/reducer.ts Outdated Show resolved Hide resolved
src/networking/index.ts Outdated Show resolved Hide resolved
@poovamraj
Copy link
Contributor Author

@frederikprijck I have improved the amount of any used now. Above all now our authentication client methods will have type support. I have also implemented all the comments you have added. Thanks for the feedback 🙇 Do check this out when you have time

src/auth/index.ts Outdated Show resolved Hide resolved
src/auth/index.ts Outdated Show resolved Hide resolved
src/auth/index.ts Outdated Show resolved Hide resolved
src/auth0.ts Show resolved Hide resolved
src/auth0.ts Show resolved Hide resolved
src/jwt/signatureVerifier.ts Outdated Show resolved Hide resolved
src/jwt/signatureVerifier.ts Outdated Show resolved Hide resolved
src/jwt/signatureVerifier.ts Outdated Show resolved Hide resolved
src/management/users.ts Outdated Show resolved Hide resolved
src/networking/index.ts Outdated Show resolved Hide resolved
poovamraj and others added 4 commits June 2, 2023 14:20
Co-authored-by: Frederik Prijck <frederik.prijck@gmail.com>
Co-authored-by: Frederik Prijck <frederik.prijck@gmail.com>
Copy link
Member

@frederikprijck frederikprijck left a comment

Choose a reason for hiding this comment

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

Added some comments, but I believe there are still some fundamental issues in ensuring TypeScript is useful here.

src/auth/index.ts Outdated Show resolved Hide resolved
src/management/users.ts Outdated Show resolved Hide resolved
src/management/users.ts Outdated Show resolved Hide resolved
@poovamraj poovamraj merged commit 881996e into vnext Jun 5, 2023
4 checks passed
@poovamraj poovamraj deleted the typescript-migration branch June 5, 2023 15:53
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.

None yet

2 participants