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

Type declarations for missing properties #51

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

janpaepke
Copy link
Contributor

This PR adds type definitions for the static values for INITIALIZING, CONNECTING, OPEN and CLOSED.
This is a backwards compatible change.

I also took the liberty of optimizing the build, since the d.ts file should be generated from the jsdoc.
Unfortunately it seems the sse.d.ts file was updated manually in 5471ff1, so it is out of sync with the source. This also means the sourcemap is inaccurate.

I fixed this as best I can, but with one caveat: The constructor (new (url: string, options?: SSEOptions): SSE;) cannot be extracted from jsdoc using tsc.
This is due to the fact that SSE is defined as a type, when it should be instance or class.
Consequently, when running tsc the constructor will be removed from the d.ts and needs to be added back in manually.
I don't think there is a workaround for this (or at least I didn't find any).

To get the output back in sync with the source, a more extensive rewrite of the types is likely due and should probably be tested using tsd or the like.

When considering such a rewrite however, my recommendation would be to refactor the code to use typescript with ecma 6 classes.
This way you could ensure compatibility with EventSource (extends EventSource) and get proper always-in-sync type definitions for free without the effort of separate definitions.

@mpetazzoni mpetazzoni merged commit 2ca2191 into mpetazzoni:main Dec 12, 2023
@janpaepke
Copy link
Contributor Author

thanks for merging.

would you consider releasing a patch version including this? :)

@mpetazzoni
Copy link
Owner

Done!

@janpaepke
Copy link
Contributor Author

janpaepke commented Jan 9, 2024

you might also want to publish the new version to npm. ;)

@mpetazzoni
Copy link
Owner

🤦🏻 Done!

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.

None yet

2 participants