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

fix: types incorrectly flag values as potentially null #670

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 21, 2024

AFAICT, every time we call .terminate() or .fingerprint(), we pass an options which cannot be nullish.
The defaultValues also have some always defined values, IMO it makes sense to report them as defined to avoid downstream code to have to use non-null-assertions.

@Acconut
Copy link
Member

Acconut commented Feb 21, 2024

Sorry, I don't fully understand. Can you share exemplary code and the errors that you are running into?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 21, 2024

Well before this PR, the types are "wrong" (or rather imprecise), e.g. tus.defaultOptions.fingerprint is typed as potentially nullish; after this PR, the types are more "correct", e.g. tus.defaultOptions.fingerprint is typed as not nullish.

@Acconut
Copy link
Member

Acconut commented Feb 21, 2024

Ok, that makes sense. I get the change for defaultOptions now, thank you. But why are you making the options argument for terminate non-nullish now? It can be null:

static terminate(url, options = {}) {

terminate can also be called by the user without any options. Am I missing something else?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 21, 2024

why are you making the options argument for terminate non-nullish now? It can be null

Maybe this one is wrong, not sure. Let's remove this change.

lib/index.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Alright, then this looks good, thank you!

@Acconut Acconut merged commit 30bfd28 into tus:main Feb 21, 2024
4 of 5 checks passed
@aduh95 aduh95 deleted the types branch February 21, 2024 15:10
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