Skip to content
This repository has been archived by the owner on Jul 14, 2022. It is now read-only.

Added TypeScript type definitions and various updates #8

Merged
merged 2 commits into from
Apr 11, 2020
Merged

Added TypeScript type definitions and various updates #8

merged 2 commits into from
Apr 11, 2020

Conversation

jef
Copy link
Contributor

@jef jef commented Feb 24, 2020

Description

Closes #7.

Took a stab at adding TypeScript type definitions with type definition tests.

Also includes:

  • GitHub Actions workflow for tests.
  • .nvmrc for those who use nvm.
  • Added semicolons. This can be removed or we can add some sort of style guide.

I'm not very partial to the other changes, but could be worth investing into.

@jef jef requested a review from btoews February 24, 2020 20:43
Copy link
Contributor

@lgarron lgarron left a comment

Choose a reason for hiding this comment

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

Taking over for @mastahyeti. :-D

LGTM with some nits.

Added semicolons. This can be removed or we can add some sort of style guide.
Would it make sense to run this through the same linter we use for the main GitHub repo?

.github/workflow/tests.yaml Show resolved Hide resolved
package.json Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@lgarron
Copy link
Contributor

lgarron commented Mar 2, 2020

@github/web-systems-reviewers Any concerns about merging this?

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

This looks like a good start. Happy to see this merged from a @github/web-systems perspective 👍

@jef
Copy link
Contributor Author

jef commented Mar 5, 2020

Okay, I think this is ready now. Let me know if you think we should add or change anything else!

@jef jef requested review from lgarron and removed request for btoews March 7, 2020 15:13
@jef
Copy link
Contributor Author

jef commented Mar 7, 2020

Would it be worth it to convert the JavaScript to TypeScript or are the typings enough?

@keithamus
Copy link
Member

I'd be keen to see this converted to TypeScript, this might be good as a follow up PR - let's get this merged first. @lgarron do you have merge capabilities? It might be useful to have @github/web-systems adopt this repo.

@ptoomey3
Copy link
Member

@lgarron - Can you re-approve and help get this pushed over the finish line if it is ready to merge? We should be able to test this pretty easily in prod/lab/etc by adding a new actions secret and verifying it is readable when run.

@lgarron
Copy link
Contributor

lgarron commented Mar 16, 2020

@lgarron - Can you re-approve and help get this pushed over the finish line if it is ready to merge? We should be able to test this pretty easily in prod/lab/etc by adding a new actions secret and verifying it is readable when run.

I actually need to ping @mastahyeti to see if he is willing to transfer the npm registration to us, which I've been putting off. :-P

Alternatively, we could fork into @github/tweetsodium. @keithamus, would that be preferable?

@keithamus
Copy link
Member

As the library already exists unprefixed on npm, we should continue publishing to that name. Let's wait for @mastahyeti to transfer ownership of the npm module to us.

@lgarron
Copy link
Contributor

lgarron commented Mar 18, 2020

As the library already exists unprefixed on npm, we should continue publishing to that name. Let's wait for @mastahyeti to transfer ownership of the npm module to us.

For the record, I emailed him but haven't heard back yet! Will wait a while before I ping him.

@lgarron
Copy link
Contributor

lgarron commented Mar 30, 2020

As the library already exists unprefixed on npm, we should continue publishing to that name. Let's wait for @mastahyeti to transfer ownership of the npm module to us.

For the record, I emailed him but haven't heard back yet! Will wait a while before I ping him.

I haven't heard back from Ben. Should we perhaps go ahead with a fork?

@ptoomey3
Copy link
Member

I haven't heard back from Ben. Should we perhaps go ahead with a fork?

Let's try one more reach-out to Ben and then decide a week or two after.

@btoews
Copy link
Contributor

btoews commented Apr 10, 2020

👋 I responded to @lgarron a few weeks ago. I added him to the NPM package and removed myself.

@btoews
Copy link
Contributor

btoews commented Apr 10, 2020

Looks like Gmail replied from a different email address than I had received the email from. Maybe that caused it to go to spam or something.

@lgarron
Copy link
Contributor

lgarron commented Apr 10, 2020

👋 I responded to @lgarron a few weeks ago. I added him to the NPM package and removed myself.

Thanks!

I do have access, I just need to add all the other JS folks who should have access before I update!

@lgarron lgarron merged commit a40f2d8 into github:master Apr 11, 2020
@lgarron
Copy link
Contributor

lgarron commented Apr 11, 2020

@jef
Copy link
Contributor Author

jef commented Apr 12, 2020

yahoo! thanks everyone for keeping this alive!

@JakeChampion
Copy link

Hi, I just noticed that the dist/index.esm.js code in 0.0.5 is using commonjs format and not esm format, is that to be expected with this change?

dist/index.esm.js code in 0.0.5 via unpkg

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TypeScript definitions
6 participants