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

Refactor to make code more TypeScript compatible #183

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

remcohaszing
Copy link
Member

This change contains some refactors, mostly to make the code better compatible with TypeScript.

All require() calls were changed to the form
const identifier = require('specifier'). This is compatible with TypeScript’s verbatimModuleSyntax option, which is strongly recommended in order to produce valid library types.

All uses of lodash were changed to native JavaScript implementations. The lodash dependency was removed.

Static class assignments were changed to static class property syntax.

util.promisify usage was replaced with Node.js promise APIs.

This change contains some refactors, mostly to make the code better
compatible with TypeScript.

All `require()` calls were changed to the form
`const identifier = require('specifier')`. This is compatible with
TypeScript’s `verbatimModuleSyntax` option, which is strongly
recommended in order to produce valid library types.

All uses of `lodash` were changed to native JavaScript implementations.
The `lodash` dependency was removed.

Static class assignments were changed to static class property syntax.

`util.promisify` usage was replaced with Node.js promise APIs.
Copy link
Collaborator

@mifi mifi left a comment

Choose a reason for hiding this comment

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

we have to be careful now that we no longer run CI tests for the supported node versions 16 and 14, that features like fs/promises, stream/promises and Object.fromEntries are avail those node versions. Or maybe we'll just say that we make this TypeScript rewrite a major version so we don't have to worry about this anymore.

Comment on lines +62 to +65
let uploadedBytes = 0
for (const l of streamLabels) {
uploadedBytes += uploadProgresses[l]
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will probably err in typescript when strict null checks is enabled. maybe better to do a streamLables.reduce(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would this result in a TypeScript error? Reduce would definitely not help with this.

I strongly believe array.reduce should never be used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the following code results in Object is possibly 'undefined'. typescript error when strict null checks is enabled (which i think makes typescript more accurate):

const progresses: Record<string, number> = { a: 1, b: 2, c:3 }
const labels = ['a', 'b', 'c']
let sum = 0;
for (const l of labels) {
  sum += progresses[l]
}

but then this also gives the same error, so ignore what i just suggested 🤦‍♂️

const sum = labels.reduce((acc, l) => acc + progresses[l], 0)

I misread your code and instead read it as a simple sum of elements. in that case this code does not produce the same ts error:

const sum = numbers.reduce((acc, n) => acc + n, 0)

but again ignore what i said.

as for using array.reduce i agree that it should be used sparingly, but as mentioned in your link it's nice for summing numbers:

It's only somewhat useful in the rare case of summing up numbers, which is allowed by default.

@remcohaszing
Copy link
Member Author

we have to be careful now that we no longer run CI tests for the supported node versions 16 and 14, that features like fs/promises, stream/promises and Object.fromEntries are avail those node versions.

I agree. I double checked all their availabilities in the docs.

Or maybe we'll just say that we make this TypeScript rewrite a major version so we don't have to worry about this anymore.

I’m ok with this, but I don’t think it’s needed. I would like to replace the uuid dependency with crypto.randomUUID. That would raise the minimal version from 14.16 to 14.17. I don’t think that’s worth making a semver major release, but I didn’t include it yet so this PR doesn’t break anything.

@mifi
Copy link
Collaborator

mifi commented Aug 29, 2024

ok, all good then!

@remcohaszing remcohaszing merged commit b581334 into main Aug 29, 2024
6 checks passed
@remcohaszing remcohaszing deleted the typescript-compat-refactors branch August 29, 2024 13: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.

2 participants