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

Enable typescript option exactOptionalPropertyTypes #245

Closed
5 tasks done
SimonSimCity opened this issue Jul 19, 2023 · 4 comments
Closed
5 tasks done

Enable typescript option exactOptionalPropertyTypes #245

SimonSimCity opened this issue Jul 19, 2023 · 4 comments
Labels
feature request A feature has been asked for or suggested by the community

Comments

@SimonSimCity
Copy link

SimonSimCity commented Jul 19, 2023

Checklist

  • I have looked into the Readme, Examples, and FAQ and have not found a suitable solution or answer.
  • I have looked into the API documentation and have not found a suitable solution or answer.
  • I have searched the issues and have not found a suitable solution or answer.
  • I have searched the Auth0 Community forums and have not found a suitable solution or answer.
  • I agree to the terms within the Auth0 Code of Conduct.

Describe the problem you'd like to have solved

Based on the conversation I've had in the typescript repository so far, it seems that #240, which originates from the changes of #235, is best resolved by enabling the typescript option exactOptionalPropertyTypes. By enabling this option, issues like #237 would already have been found by running the typescript compiler, which is done prior to every release of the code here.

Does anything speak against enabling this option, or is there a reason you would not like to do it?

Describe the ideal solution

Enable the typescript option exactOptionalPropertyTypes and revert the changes done in #243.

Alternatives and current workarounds

Keep the changes done in #243 and leave exactOptionalPropertyTypes disabled.

Additional context

microsoft/TypeScript#55058

@SimonSimCity SimonSimCity added the feature request A feature has been asked for or suggested by the community label Jul 19, 2023
@frederikprijck
Copy link
Member

frederikprijck commented Jul 19, 2023

Thanks, can you elaborate why you believe we should revert the PR? I'm not sure I understand.

@frederikprijck
Copy link
Member

frederikprijck commented Jul 19, 2023

Regardless of the issue you reported (#240) , there is another issue caused by readonly which isnt solved by exactOptionalPropertyTypes :

[!] (plugin rpt2) Error: /Users/frederikprijck/Development/auth0-vue/src/plugin.ts(74,3): semantic error TS2416: Property 'idTokenClaims' in type 'Auth0Plugin' is not assignable to the same property in base type 'Auth0VueClient'.
  Type 'Readonly<Ref<{ readonly [x: string]: any; readonly __raw: string; readonly name?: string; readonly given_name?: string; readonly family_name?: string; readonly middle_name?: string; readonly nickname?: string; ... 31 more ...; readonly org_name?: string; } | undefined>>' is not assignable to type 'Ref<IdToken | undefined>'.
    Types of property 'value' are incompatible.
      Type '{ readonly [x: string]: any; readonly __raw: string; readonly name?: string; readonly given_name?: string; readonly family_name?: string; readonly middle_name?: string; readonly nickname?: string; ... 31 more ...; readonly org_name?: string; } | undefined' is not assignable to type 'IdToken | undefined'.
        Type '{ readonly [x: string]: any; readonly __raw: string; readonly name?: string; readonly given_name?: string; readonly family_name?: string; readonly middle_name?: string; readonly nickname?: string; readonly preferred_username?: string; ... 30 more ...; readonly org_name?: string; }' is not assignable to type 'IdToken' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
          Types of property 'amr' are incompatible.
            The type 'readonly string[]' is 'readonly' and cannot be assigned to the mutable type 'string[]'.
src/plugin.ts

The above is when compiling our SDK itself, not when consuming it.
We could have solved that differently, but I felt like readonly causes more issues that it solved, which is why we got rid of it.

Does anything speak against enabling this option, or is there a reason you would not like to do it?

I'm mostly interested in why we should enable it. I'm happy to do so if I can understand what it would solve now that readonly is removed.

@SimonSimCity
Copy link
Author

Thanks for your time @frederikprijck! Suggesting that the PR could be reverted was by assuming it was only because of difference it makes in the generated types, like I reported in #240. I'll remove it from the initial suggestion.

Enabling the option exactOptionalPropertyTypes in this typescript project would make that other projects (those having exactOptionalPropertyTypes enabled) can rely on that the types are correct in regards to undefined. Leaving this option disabled, you (in this project) could assign undefined to the property username of an instance of class User { username? = '' }, which would be not allowed and unexpected in a project having exactOptionalPropertyTypes enabled, like I have in mine. If the function readonly() would not have changed anything on the readability of types, exactOptionalPropertyTypes would've made the difference. See the d.ts file this imaginary example:

https://www.typescriptlang.org/play?exactOptionalPropertyTypes=false&ts=5.0.4#code/KYDwDg9gTgLgBAE2AYwDYEMrDgMwK4B2yMAlhAYsMGAMIRgCeAPACpygzAEIDOcEAIwBWKGAD4AFDEwBzYDABccFgEolAESq16zFmIDcAKFCRYlNJmwwGYbJup1GrMXAC8y9iE7c+AbwC+cAD8cL6GcBFwANoA0nAkFADWwAwQOMoAukossRlGgdlGxuDQ8EgWWHAWPHwAqjzAUKHhkehBSjwwUAkyRpFwAh1dPfmGxabwyOSdcHgNTe5IDjoSBMAA7nD1jRIqcCqGQA

... and compare it to the result of when having exactOptionalPropertyTypes enabled:

https://www.typescriptlang.org/play?exactOptionalPropertyTypes=true&ts=5.0.4#code/KYDwDg9gTgLgBAE2AYwDYEMrDgMwK4B2yMAlhAYsMGAMIRgCeAPACpygzAEIDOcEAIwBWKGAD4AFDEwBzYDABccFgEolAESq16zFmIDcAKFCRYlNJmwwGYbJup1GrMXAC8y9iE7c+AbwC+cAD8cL6GcBFwANoA0nAkFADWwAwQOMoAukossRlGgdlGxuDQ8EgWWHAWPHwAqjzAUKHhkehBSjwwUAkyRpFwAh1dPfmGxabwyOSdcHgNTe5IDjoSBMAA7nD1jRIqcCqGQA

The reason for this change in types is explained here: microsoft/TypeScript#55058 (comment)

@frederikprijck
Copy link
Member

frederikprijck commented Jul 19, 2023

Thanks for that. I have no objections to add it, but I would like to familiarize myself more with the implications before doing so. Even more so, given I don't see any direct need I would prefer not to prioritze this at the moment.

If you believe this does need to be added to fix things, happy to reconsider!

Closing this for now, but thanks for bringing this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A feature has been asked for or suggested by the community
Projects
None yet
Development

No branches or pull requests

2 participants