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

Make options optional no default function parameter initializer (BETA) #179

Merged
merged 4 commits into from
Aug 1, 2023
Merged

Make options optional no default function parameter initializer (BETA) #179

merged 4 commits into from
Aug 1, 2023

Conversation

cristobal
Copy link
Contributor

Rather than have the default parameter with an default initializer should for when the provided value is undefined, it could be better to have it as an optional object. Thereby the initializer statement options = options || {}; is much clearer.

closes #177

@cristobal cristobal requested a review from a team as a code owner July 31, 2023 19:49
@cristobal cristobal changed the title Make options optional no default function initializer (BETA) Make options optional no default function parameter initializer (BETA) Jul 31, 2023
lib/index.ts Outdated Show resolved Hide resolved
token: string,
options: JwtDecodeOptions = { header: false }
) {
function jwtDecode(token: string, options?: JwtDecodeOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is wrong with providing a default? I am not sure I follow.

Copy link
Member

@frederikprijck frederikprijck Jul 31, 2023

Choose a reason for hiding this comment

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

By using the default, the line here seems like dead-code. I want to leave that line because of people passing in null and not using anything that tells them it shouldnt. We could consider ensuring the types support null as well, but there shouldn't be no need for people to pass in null, so I'd rather not signal that in the public API.

By removing the default, that line is now also covered when passing in undefined (or nothing), making it no longer dead-code when looking at it from a types point of view.

I don't mind adding in that change, as I introduced the overload in the beta to begin with, and I believe it may not have been 100% accurate.

Copy link
Contributor

@jonkoops jonkoops Jul 31, 2023

Choose a reason for hiding this comment

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

Hmmm, yeah I see. The current types that are already in a published version also marked this as optional, so the types should remain the same.

I guess this is a question of whether the library should do all this validation, or if that is the job of TypeScript. And if users don't use TypeScript, should we handle all these edge cases of input validation in code?

I'd argue that if users need to validate their input is correct, that they should adopt TypeScript. Especially considering we'd essentially be shipping more code, such as the run-time type checks to the user, even if they are running in production. I'd even argue we should remove the existing checks for specific types, and make a clean break to let TypeScript handle such things.

Copy link
Member

Choose a reason for hiding this comment

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

Thie library didnt use TypeScript before v4, and we introduced TypeScript for convenience and easier tool alignment.

We don't want to make any assumptions on our users using typescript, that's not the scope of the new major.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup i'm also accustomed to rely on TypeScript or intellisense / code completion from the editors. So as you say @jonkoops most people should get an warning should they ever try to pass null in an editor that uses intellisense and relies on the types.

However as @frederikprijck says here, there may be people out there that are passing null an won't get an warning when upgrading, should we had rely on the types only we should have then perhaps added an assert instead that checks if the passed input is null an throw an error instead.

This is an easy fix and we retain the same logic with the options = options || {} initializer as before.

Copy link
Contributor Author

@cristobal cristobal Aug 1, 2023

Choose a reason for hiding this comment

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

Thie library didnt use TypeScript before v4, and we introduced TypeScript for convenience and easier tool alignment.

We don't want to make any assumptions on our users using typescript, that's not the scope of the new major.

Users that use an editor with intellisense / code completion that leverages typescript definitions file would have gotten an error/warning if they passed null as an argument does not matter if they are using TypeScript or not. Since most modern editors have some basic type checking built in by default or in a JS environment where a JavaScript LSP plugin is used.

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