Enable exactOptionalPropertyTypes in TypeScript configuration
#6040
Conversation
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
|
Murderlon
left a comment
Murderlon
left a comment
There was a problem hiding this comment.
I think I'm against this change, it's very verbose for no benefit at all. We have to put up with this in our entire codebase while there is only benefit for the user facing types, not all the code within.
There was a problem hiding this comment.
exactOptionalPropertyTypes is now recommended by typescript so I agree it makes sense to enable it.
Also, I see that we don't currently have any large pending PRs that would cause a merge conflict with this, so I think it's ok to make the change now.
See my comments about adding null values and simplifying the explicit undefined checks
| const body = opts.formData | ||
| ? this.createFormDataUpload(file, opts) | ||
| : file.data | ||
| : (file.data ?? null) |
| file: UppyFile<M, B>, | ||
| chunk: Chunk, | ||
| signal?: AbortSignal, | ||
| signal: AbortSignal | null = null, |
There was a problem hiding this comment.
I see that you add null as an option many places. why not undefined? null seems more like an explicit value to me, rather than an absent one.
| getChunkSize: this.opts.getChunkSize | ||
| ? this.opts.getChunkSize.bind(this) | ||
| : undefined, | ||
| ...(this.opts.getChunkSize !== undefined && { |
There was a problem hiding this comment.
I think in the cases where the optional value is an object or a function, we can simplify these, so it doesn't looks so long and ugly:
| ...(this.opts.getChunkSize !== undefined && { | |
| ...(this.opts.getChunkSize && { |
| } else if (this.#isRestoring) { | ||
| this.options.companionComm.restoreUploadFile(this.#file, { | ||
| uploadId: this.options.uploadId, | ||
| ...(this.options.uploadId !== undefined && { |
There was a problem hiding this comment.
here i agree we should use the !== undefined check because the string could be '' which we probably want to include
| companionHeaders: this.opts.companionHeaders, | ||
| companionKeysParams: this.opts.companionKeysParams, | ||
| companionCookiesRule: this.opts.companionCookiesRule, | ||
| ...(this.opts.companionHeaders !== undefined && { |
There was a problem hiding this comment.
for example these we could also simplify
| ...(this.opts.companionHeaders !== undefined && { | |
| ...(this.opts.companionHeaders && { |
3 participants
This will help prevent regressions similar to #6033 in the future.