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

JSDoc style comments create typescript types #42

Merged
merged 11 commits into from
Nov 8, 2023

Conversation

dedalusMohantyMa
Copy link

@dedalusMohantyMa dedalusMohantyMa commented Oct 16, 2023

I added JSDoc annotations that can create typescript types.

If you run the script in package.json generate-types it will genereate types into the types directory.
I hope I wrote them correctly, so kindly @dedalusMohantyMa , when you see something fishy 🐟

Kindly review this, as this is my first time doing this :-)

#32

@dedalusMohantyMa dedalusMohantyMa changed the title JSDoc style comments create typescript types DRAFT: JSDoc style comments create typescript types Oct 16, 2023
@dedalusMohantyMa dedalusMohantyMa changed the title DRAFT: JSDoc style comments create typescript types JSDoc style comments create typescript types Oct 17, 2023
@Barsoomx
Copy link

Barsoomx commented Nov 1, 2023

@dedalusMohantyMa
You missed readyState property typedef which is widely used in EventSource, so i think it's crucial it is defined

@mpetazzoni mpetazzoni merged commit f483e8e into mpetazzoni:main Nov 8, 2023
Comment on lines +44 to +65
export type SSEOptions = {
/**
* - headers
*/
headers: SSEHeader;
/**
* - payload as a string
*/
payload: string;
/**
* - HTTP Method
*/
method: string;
/**
* - flag, if credentials needed
*/
withCredentials: boolean;
/**
* - debugging flag
*/
debug: boolean;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

@mpetazzoni Are these properties all mandatory? If not, you'll want to add ? behind the property name.

We get this error, because we don't pass method and debug:

error TS2345: Argument of type '{ headers: { "Content-Type": string; }; withCredentials: true; payload: string; }' is not assignable to parameter of type 'SSEOptions'.
  Type '{ headers: { "Content-Type": string; }; withCredentials: true; payload: string; }' is missing the following properties from type 'SSEOptions': method, debug

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened a PR to fix these: #43.

Copy link

Choose a reason for hiding this comment

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

Yes, these properties should not be required as they all have default values supplied:

sse.js/lib/sse.js

Lines 30 to 35 in cdb9a03

options = options || {};
this.headers = options.headers || {};
this.payload = options.payload !== undefined ? options.payload : '';
this.method = options.method || (this.payload && 'POST' || 'GET');
this.withCredentials = !!options.withCredentials;
this.debug = !!options.debug;

Copy link
Author

Choose a reason for hiding this comment

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

yeah sry, I was preoccupied with other work. Thanks for the fix ❤️

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.

6 participants