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.

Sign up for GitHub

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

Jump to bottom

Typescript sdk standardization #700

Merged
merged 6 commits into from
Feb 26, 2025
Merged

Typescript sdk standardization #700

merged 6 commits into from
Feb 26, 2025

Conversation

Superpat
Copy link
Contributor

Fixes a bug in my last pr that was pointed out by @nickchomey and closes #603

I decided to keep both api's since it was pretty simple.

Maybe wait till @nickchomey has reviewed this since he found out the flaw in the last pr.

@nickchomey
Copy link

Seems ok to me!

@bencroker
Copy link
Collaborator

Thank you both!

While reviewing this I noticed that you’ve hard-coded the Datastar version number in node.ts and deno.ts. Can you look please into importing the version number instead?

datastar/sdk/typescript/src/node/node.ts

Line 13 in a4c2e87

`<html><head><script type="module" src="https://cdn.jsdelivr.net/gh/starfederation/[email protected]/bundles/datastar.js"></script></head><body><div id="toMerge" data-signals-foo="'World'" data-on-load="@get('/merge')">Hello</div></body></html>`,

datastar/sdk/typescript/src/web/deno.ts

Line 10 in a4c2e87

`<html><head><script type="module" src="https://cdn.jsdelivr.net/gh/starfederation/[email protected]/bundles/datastar.js"></script></head><body><div id="toMerge" data-signals-foo="'World'" data-on-load="@get('/merge')">Hello</div></body></html>`,

@Superpat
Copy link
Contributor Author

Right, good point

@Superpat
Copy link
Contributor Author

@nickchomey I've done some changes to the comments, does that make keepalive more clear to you ?

@nickchomey
Copy link

Thanks, those comments are helpful re: keepalive.

Though, perhaps before merging this, let me dig into the sdk today and report back later with any further feedback

@nickchomey
Copy link

Ive done a bit of testing and it seems fine. I'll be sure to report any issues or improvements if I find them

@bencroker
Copy link
Collaborator

@Superpat please resolve the conflicts and report the current status.

@Superpat
Bring typescript sdk in line with sdk api's
61011eb 
But still allow passing of stricter types when it makes sense
@Superpat
fix bug introduced in starfederation#698
434189c 
we want the stream to be ended only if keepalive is not present
@Superpat
Remove close method from typescript sdk and improve comments
1635363 
The stream is closed automatically after the callback runs.
The only situation where we want to use keepalive is if we
want an infinite stream, in which case we dont want to close it.
@Superpat
improve package.json for typescript sdk
fab19c8
@Superpat
Use consts VERSION in typescript sdk tests
cb88cc3
@Superpat
Improve method comments in typescript sdk
8ed5bd5 
this sdk is also used in javascript where there's less type information
@Superpat Superpat force-pushed the typescript-sdk-standardization branch from 8be3710 to 8ed5bd5 Compare February 26, 2025 12:18
@Superpat
Copy link
Contributor Author

@bencroker done

@bencroker
Copy link
Collaborator

Merged, thank you both!

@bencroker bencroker merged commit 14dab8a into starfederation:develop Feb 26, 2025
1 check passed
@Superpat Superpat deleted the typescript-sdk-standardization branch February 26, 2025 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bencroker bencroker bencroker approved these changes

@delaneyj delaneyj Awaiting requested review from delaneyj delaneyj is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bring base typescript sdk in line with spec
3 participants
@Superpat @nickchomey @bencroker