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

Add typescript support #55

Merged
merged 1 commit into from
Sep 11, 2021
Merged

Add typescript support #55

merged 1 commit into from
Sep 11, 2021

Conversation

aeharding
Copy link
Contributor

Hello!

Thanks for considering this PR. 🙂

Some things of note:

  1. Prettier on my editor formats code in the readme. Let me know, and I can revert + add to .pretterignore if this is not wanted.
  2. I had to remove standard.js, as its not compatible with typescript. I just used the react default. lmk and I can change this to something else! (ts-standard?)
  3. withServiceWorkerUpdater was hard to type (there's a couple as) but the API for the end user is nice :)

I've tested the example/ folder, along with a separate app (via npm link). Hopefully nothing seems too off.

And if you'd like, feel free to push changes directly to my branch.

}

export interface WithServiceWorkerUpdaterOptions {
message?: unknown
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if there's a specific type this should be associated with.

Copy link
Owner

Choose a reason for hiding this comment

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

It looks pretty unknown (any type of simple or complex data, but not functions or classes). I'm not TypeScript expert and maybe exists something specific for this.

@aeharding aeharding changed the title Add typescript support (#54) Add typescript support Sep 4, 2021
@emibcn emibcn linked an issue Sep 11, 2021 that may be closed by this pull request
@emibcn emibcn added enhancement New feature or request TypeScript TypeScript knowledge needed labels Sep 11, 2021
@emibcn emibcn merged commit e9e5596 into emibcn:main Sep 11, 2021
@emibcn
Copy link
Owner

emibcn commented Sep 11, 2021

Hi @aeharding !!
Great job!! I have done several tests (apart from the CI ones, on side projects) and it continues working well, so I approved and merged this PR.

I'm about to publish a new version of the NPM package, but I'm in doubt about the version number to assign. Do you think this is a minor change (1.1.0) or it deserves considering it as a major one (2.0.0)?

Thanks for your contributions!!

@aeharding
Copy link
Contributor Author

Thank you!! Hmm, good question. I guess strictly looking at semantic versioning it would be a minor version change, since it's fully backwards compatible without breaking changes to Javascript projects.

But 2.0.0 might be more clear to typescript users that 1.x isn't compatible (in case a TS user downloads 2.0.0 but wants to try an older version for whatever reason.)

Either way seems fine by me :)

@emibcn
Copy link
Owner

emibcn commented Sep 11, 2021

Published with major version change ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request TypeScript TypeScript knowledge needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript support
2 participants