-
Notifications
You must be signed in to change notification settings - Fork 247
Fix TS 5.7 generic Uint8Array declaration when inferred #1668
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
Conversation
🦋 Changeset detectedLatest commit: d0f4ff4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
| "format:check": "prettier --check src examples/**/*.ts", | ||
| "ci:publish": "pnpm build:clean && pnpm compat && changeset publish", | ||
| "downlevel-dts": "downlevel-dts ./dist/ ./dist/ts4.2 --to=4.2", | ||
| "downlevel-dts": "downlevel-dts ./dist/src ./dist/ts4.2 --to=4.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive by fix that ensures we really only downlevel the emitted d.ts files in the dist/src folder where the original source types are emitted to.
…-js into lukas/fix-ts-incompat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's here makes sense to me!
That being said, as is this change doesn't do anything to prevent this from happening in the future, to me it seems like without some guardrails, this will inevitably regress.
I realize this might be a bit of a tedious ask, but maybe it would be worth adding to CI something like:
- Build the library normally.
- Install the oldest supported typescript version.
- Use that oldest supported typescript version to build a script containing something like
import 'livekit-client'. - If step 3 it returns a non-zero code, fail the CI run.
Thoughts on that?
|
Yeah, I like the suggestion of an additional CI test. Just realised that the issue is not actually about 5.9 (which raises some build errors also internally for us) but is likely due to the changes in TS 5.7 when the Uint8Array was first made generic. I'll update the description to reflect that |
…e works when compiled against old typescript versions More context can be found here: #1668 (review)
closes #1661
So far we’ve kept pretty much up to date with TS versions and used
downlevel-dtsto transpile any non compatible TS constructs down for older versions. This CLI tool hasn't been updated in a while though, so this particular issue at hand is not part of the fixes it supports.When a
Uint8Arraylike type is inferred directly from usage within a TS >5.7 project (like this one) then the emittedd.tswill use something likeUint8Array<ArrayBuffer>which is incompatible with older TS versions.The options we have is
Uint8Arrayas return type (or declared type)ReturnType<Uint8Array.from>I opted for the explicit declaration for now.
full context: microsoft/TypeScript#62240