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.

Sign up for GitHub

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

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a21042c
make options optional
cristobal Jul 31, 2023
2bd3bb9
Update index.ts
cristobal Jul 31, 2023
26ccff0
remove comment
cristobal Aug 1, 2023
78f179c
Merge branch 'beta' into patch-3
frederikprijck Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@ function jwtDecode<T = JwtHeader>(
options: JwtDecodeOptions & { header: true }
): T;
function jwtDecode<T = JwtPayload>(token: string, options?: JwtDecodeOptions): T;
function jwtDecode(
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

if (typeof token !== "string") {
throw new InvalidTokenError("Invalid token specified: must be a string");
}

// default empty options object when the provided options is either null | undefined
cristobal marked this conversation as resolved.
Show resolved Hide resolved
options = options || {};
const pos = options.header === true ? 0 : 1;

Expand Down