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 options initializer (BETA) #177

Closed
wants to merge 1 commit into from
Closed

Remove options initializer (BETA) #177

wants to merge 1 commit into from

Conversation

cristobal
Copy link
Contributor

@cristobal cristobal commented Jul 31, 2023

There is no need to have the options initializer, since it will already be initialized by the default parameter in the function should undefined be provided.

This is dead code that never will incur.

@cristobal cristobal requested a review from a team as a code owner July 31, 2023 17:38
@cristobal cristobal changed the title Remove options initialize (BETA) Remove options initializer (BETA) Jul 31, 2023
@frederikprijck
Copy link
Member

frederikprijck commented Jul 31, 2023

According to mdn, we do need this.

Default function parameters allow named parameters to be initialized with default values if no value or undefined is passed.

Null is neither undefined or not a value, so that would not use the default.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/default_parameters#passing_undefined_vs._other_falsy_values

@cristobal
Copy link
Contributor Author

Yes 🤦🏽

@cristobal cristobal closed this Jul 31, 2023
@cristobal cristobal reopened this Jul 31, 2023
@cristobal
Copy link
Contributor Author

cristobal commented Jul 31, 2023

Still the why for this PR here is that the type defintions do not allow for null as an input value to the function. It should either be an object or undefined.

Yes people can still call with null if they want when in environments where they are not using code completion.

Dunno an alternative here would be to assert for null an throw an error If that’s the case, since the type definitions are either expecting an object or undefined.

@cristobal
Copy link
Contributor Author

cristobal commented Jul 31, 2023

Or perhaps it would be better to just have options?: JwtDecodeOptions, then the default initializer statement is more explicit on why it's there as it's now it's a bit confusing.

Created a PR on this #179

@jonkoops
Copy link
Contributor

Yes people can still call with null if they want when in environments where they are not using code completion.

This is always possible, there should be no need to check for this. If folks want to be type-safe they can opt in to TypeScript. Otherwise we'd end up with a bunch of defensive code that is shipped to all users (some of which already exists).

@frederikprijck
Copy link
Member

Closing, as I believe we want to keep this. Even though I agree with what @jonkoops said, the code allowed for null to be passed so let's keep that.

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