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

Fixes the types related to event types. #916

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Jan 29, 2024

Thanks to @CITGuru for reporting this! It closes #914 and #915.

Copy link

github-actions bot commented Jan 29, 2024

Size Change: 0 B

Total Size: 11.4 MB

ℹ️ View Unchanged
Filename Size
dist/stellar-sdk.js 6.31 MB
dist/stellar-sdk.min.js 5.08 MB

compressed-size-action

@Shaptic Shaptic mentioned this pull request Jan 29, 2024
4 tasks
@CITGuru
Copy link

CITGuru commented Jan 30, 2024

@Shaptic great that helps. However, the EventSource static variable type declaration is not backward compatible. If you install this in Typescript less than 5, it throws the same error. Typescript changed from using number in older versions to using actual numbers in newer versions.

@Shaptic
Copy link
Contributor Author

Shaptic commented Jan 30, 2024

Ah, so this is a breaking change, then... 🤦 did TypeScript break this across a minor version boundary, then??

@Shaptic
Copy link
Contributor Author

Shaptic commented Jan 30, 2024

But then doing something like number | 1 means downstream needs to handle the union 🤔 maybe I'll just include this in the next major version release? though that may not be for some time... this is obnoxious to say the least. What are your thoughts as a downstream user, @CITGuru? Is requiring TypeScript 5 unreasonable?

@CITGuru
Copy link

CITGuru commented Jan 30, 2024

But then doing something like number | 1 means downstream needs to handle the union 🤔 maybe I'll just include this in the next major version release? though that may not be for some time... this is obnoxious to say the least. What are your thoughts as a downstream user, @CITGuru? Is requiring TypeScript 5 unreasonable?

Yes, this would affect downstream users and projects who have not yet upgraded to Typescript or just can't. I had to downgrade one of our projects back to Typescript 4 as I didn't have enough bandwidth to fix all the issues as it affected other libraries still using old Typescript.

Adding union to support number | 1 might work.

@Shaptic
Copy link
Contributor Author

Shaptic commented Feb 13, 2024

Hey @CITGuru just want to call out this note from the latest release (#918):

There will come a future major release in which we drop support for TypeScript 5 and other outdated tooling that will incorporate this change with enough warning to downstream systems.

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.

EventSource in (dom-monkeypatch.d.ts) is conflicting with EventSource in Typescript lib.dom.d.ts
2 participants