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

Fixup tsc build breakage in test_coverall2.ts #168

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

jhugman
Copy link
Owner

@jhugman jhugman commented Dec 2, 2024

Fixes #167

According to The Big O of Code Reviews, this is a O(n) change.

The build was breaking due to a compilation error in the Typescript test, due to a change in Typescript versions.

You can see the code in this Typescript playground: updating to Typescript v5.7.2 introduces the compile time error.

The fix presented here is the simplest dumbest fix: we no longer are relying on enumerating the type constructors, we now enumerate transitional objects that we provide, with hand written calls into a constructor.

@jhugman jhugman requested review from zzorba and Johennes December 2, 2024 12:31
@Johennes
Copy link
Collaborator

Johennes commented Dec 2, 2024

It looks like TS 5.7.2 made the TypedArray types generic. The generic parameter has a default argument that should commonly spare callers from having to specify it. It does appear to be needed in this case, however. When I add it on the first array element in your playground, the code appears to transpile correctly.

  for (const TypedArray of [
    Uint8Array<ArrayBuffer>, <==
    Uint16Array,
    Uint32Array,
    ....

@jhugman
Copy link
Owner Author

jhugman commented Dec 2, 2024

That's super interesting. Thanks for digging that out!

@Johennes As the reviewer and official typescript wonk :), would you prefer I add the type parameter (now breaks in v5.6.3), or my dumb re-write that works in both?

@Johennes
Copy link
Collaborator

Johennes commented Dec 2, 2024

Wait, I think I'm slightly confused. Do we deliberately support different TS versions? Could we not lock in a specific version by creating a lock file from package.json and then use that when running the tests?

Copy link
Collaborator

@zzorba zzorba left a comment

Choose a reason for hiding this comment

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

LGTM

@jhugman jhugman merged commit 189decf into main Dec 2, 2024
17 checks passed
@jhugman jhugman deleted the jhugman/167-test_coverall2_typescript branch December 2, 2024 15:26
@jhugman
Copy link
Owner Author

jhugman commented Dec 2, 2024

Sorry @Johennes , I was over hasty merging this without responding to your comment.

Do we deliberately support different TS versions? Could we not lock in a specific version by creating a lock file from package.json and then use that when running the tests?

We currently don't have any nodejs dependencies, just devDependencies, so yes, I think we could do this.

My understanding was that it wasn't a great idea to keep a lock file in version control for libraries. I am very easily persuaded one way or the other, since a) the yarn.lock files for libraries seem to be ignored when building the app, b) this is only for tests (at the moment).

What about Cargo.lock? Well, ubrn is the application and we don't provide any Rust that runs in the user's apps, so I think we should have a Cargo.lock.

@Johennes
Copy link
Collaborator

Johennes commented Dec 3, 2024

No worries at all. Merging this was the right thing to do as it unbreaks the CI. Sorry, I should have approved first and discussed potential future improvements second.

It's interesting that the person in that GitHub issue recommends not to include yarn.lock in version control when the official yarn docs state the opposite. 😵‍💫

Now that we have no failures anymore, it's not a real problem. I've put in #170 to discuss and possibly take action some time in the future.

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.

test_coverall2.ts(120,49): error TS2554: Expected 0-1 arguments, but got 3.
3 participants