Try to build types with a constructor

[mpetazzoni]

@janpaepke Following your PR #51, I've tried to clean up the types some more in a way that does produce a constructor. But honestly I no close to nothing about JSDoc and I have no idea if what is produced is correct and will work. Would you mind taking a look at the changes below?

Thanks!

[dedalusMohantyMa]

I will try it out, if I have some time this week.

[janpaepke]

Sorry, this must have slipped past. Can I still help here?

[mpetazzoni]

@janpaepke Yes! This might need to be updated based on the recent changes though, but if you could take a crack at it I would very much appreciate it!

[janpaepke]

I could have a crack at the types, but since time is limited I would rather not dig into other changes. Can you update this so I have a current version to start from?

[dedalusMohantyMa]

So I tried it out, and what misses are the onMessage etc. functions, which are declared as a callback.

[janpaepke]

Are you saying those are the only thing missing to have a working/finished PR or are you saying those are missing for it to be an up to date basis for me to start working on it?

[mpetazzoni]

@janpaepke I took a crack at updating the types. Can you see if those work?

[janpaepke]

All right, I had a look and unfortunately the issue is not resolved.

As roughly outlined in #51, the problem is that the sse.d.ts file can't be directly generated from the source lib. You should never have to edit the sse.d.ts manually (unless you have no typedefs at all in your source), because it makes things unmaintainable. The lib source should be the single source of truth (duh).
In fact the typedefs shouldn't even be part of the repo and only part of the released package as part of a /dist folder or the like.

That being said, running npm run build (or tsc), modifies the .d.ts file compared to the committed version. Most notably it removes the constructor. As also mentioned in #51, the reason is that types are not meant to have one and (to my knowledge) there is no way to trick jsdoc into picking it up.

The correct way of resolving this (imho) is to define the exported SSE object as a class rather than a function. This way jsdoc would understand to include a constructor.

But to be honest, the optimal way would be to refactor this to typescript. There are already so many type annotations in there that this would actually help with legibility. And it would give correct .d.ts exports for 'free'.

Since there are comprehensive tests, I could confidently take a crack at this and probably knock something out in a few hours. I would start from main, though, since the tests are actually failing in this branch.

Would you like me to?

[janpaepke]

Had some time after lunch and did this: https://github.com/janpaepke/sse.js/tree/chore/ts-refactor

Check out the changes here. It's now fully TS, it builds to js, all tests work and I tried using it in our app, where it also behaves as expected. The types look correct, too.

When porting this I made an effort to preserve current functionality, even though the stricter type system revealed several contradictions and even potential bugs, most notably around events. The one thing that will not work anymore is using const sse = SSE(...) rather than const sse = new SSE(...), which you shouldn't do anyway, but still making this a breaking change.

For people preferring "the functional way" we could consider exporting a createSSE function.

Before releasing this, should you want to continue down this path, we need to:

1. Adress the event type issues
2. Adress other issues and add more tests
3. Add rollup to build for both ESM and CJS
4. Update Readme

[janpaepke]

@mpetazzoni would you like to continue with this?

[mpetazzoni]

@janpaepke Yes! Thanks for this great work. Sorry I haven't had time to look at it closely yet though - just had a baby 😅

[janpaepke]

Oh wow, congratulations. Well I've been there. =)

Take your time and we'll revisit this once you had some sleep.