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

Install and use the most recent Node types for the types tests #19000

Merged
merged 1 commit into from
Nov 3, 2024

Conversation

timvandermeij
Copy link
Contributor

The types tests run in Node.js and therefore use Node types for e.g. builtins. However, we didn't explicitly indicate this in tsconfig.json (see [1] for more information and [2] for the PR where we found this). Moreover, we didn't explicitly install the most recent version of @types/node which implicitly made us fall back to version 14.14.45 (because that was installed as a dependency of other modules) whereas much newer versions are available and we need those after changes in Node.js (see [3] for more information and [4] for the PR where we found this).

This commit fixes both issues by explicitly installing and using the most recent Node.js types, which should also avoid future issues with the types tests.

[1] TypeStrong/ts-node#1012
[2] #18237
[3] https://stackoverflow.com/questions/78790943/in-typescript-5-6-buffer-is-not-assignable-to-arraybufferview-or-uint8arr
[4] #18959

The types tests run in Node.js and therefore use Node types for e.g.
builtins. However, we didn't explicitly indicate this in
`tsconfig.json` (see [1] for more information and [2] for the PR where
we found this). Moreover, we didn't explicitly install the most recent
version of `@types/node` which implicitly made us fall back to version
14.14.45 (because that was installed as a dependency of other modules)
whereas much newer versions are available and we need those after
changes in Node.js (see [3] for more information and [4] for the PR
where we found this).

This commit fixes both issues by explicitly installing and using the
most recent Node.js types, which should also avoid future issues with
the types tests.

[1] TypeStrong/ts-node#1012
[2] mozilla#18237
[3] https://stackoverflow.com/questions/78790943/in-typescript-5-6-buffer-is-not-assignable-to-arraybufferview-or-uint8arr
[4] mozilla#18959
@timvandermeij
Copy link
Contributor Author

I have confirmed locally that this change works for the current master branch and on top of #18959.

@Snuffleupagus Could you also test this with your patch, since I think this should allow removing the types test specific changes from your PR (the downgrade of TypeScript and the tsconfig.json change)?

@Snuffleupagus
Copy link
Collaborator

Could you also test this with your patch, since I think this should allow removing the types test specific changes from your PR (the downgrade of TypeScript and the tsconfig.json change)?

It seems to work just fine locally, thank you!

However, I do wonder if we shouldn't also downgrade TypeScript a little bit since it seems that using the very latest version often causes trouble (for end-users of pdfjs-dist) in practice?
A recent example is issue #18867, but I do recall older issues as well that often seems to be connected with us always using the latest TypeScript version; could we e.g. settle for a version that's between 6 and 12 months old?

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Nov 3, 2024

I'm a bit on the fence about that. I personally prefer that we use the most recent version: it includes support for e.g. modern JS syntax/features that we use in PDF.js itself, which with older versions could become an issue for us if we start using newer functionality that the older version doesn't support yet, but it also includes bugfixes and security patches which is relevant for keeping the npm audit output clean.

I'm also not really sure what version we should use then since in general we have seen that users have a very different pace for updating dependencies, so while using a slightly older version might help for one group of users it's unlikely to be enough for other groups of users that run even older versions or different combinations of dependencies.

Considering this, I think that using the most recent version is better, at least for now, and it also provides users with an incentive to keep dependencies up-to-date. It should obviously be within reason: if a newer version causes significant breakage, then obviously we can downgrade it at that time for a reasonable amount of time (but I'd prefer we only do that if there are multiple reports about it, and then we also have good reason for doing so).

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Well, I can't really argue with the points raised in #19000 (comment) :-)

r=me, thank you.

@timvandermeij timvandermeij merged commit 20fbb4d into mozilla:master Nov 3, 2024
8 checks passed
@timvandermeij timvandermeij deleted the types-node branch November 3, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants