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

feat: all the things! #1

Merged
merged 8 commits into from
Aug 31, 2018
Merged

feat: all the things! #1

merged 8 commits into from
Aug 31, 2018

Conversation

soldair
Copy link
Contributor

@soldair soldair commented Aug 29, 2018

types for npm related objects!

@soldair soldair changed the title chore: initial setup WIP chore: initial setup Aug 29, 2018
@soldair soldair changed the title WIP chore: initial setup chore: initial setup Aug 30, 2018
@soldair
Copy link
Contributor Author

soldair commented Aug 30, 2018

ping @chrisdickinson
i think this is ready to merge/review =)

/cc @ofrobots

@soldair soldair changed the title chore: initial setup feat: all the things! Aug 30, 2018
Copy link
Contributor

@chrisdickinson chrisdickinson left a comment

Choose a reason for hiding this comment

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

❤️ This is great! 1 Q: would you mind if I rename Person to Maintainer?

@soldair
Copy link
Contributor Author

soldair commented Aug 31, 2018

i will fix this. perfect suggestion.

I was also wondering i think there is a third type of document? pacote has a method called "manifest" i wanted to make sure that AbbreviatedDocument (corgi) is not also the manifest cached by pacote

@soldair
Copy link
Contributor Author

soldair commented Aug 31, 2018

let me know if there is anything else you think i should do before you publish =)

@zkat
Copy link

zkat commented Aug 31, 2018

@soldair pacote.manifest() returns corgis by default, and the full manifest if you do opts = { fullMetadata: true }

@soldair
Copy link
Contributor Author

soldair commented Aug 31, 2018

oh thanks @zkat
in that case i would like to rename AbbreviatedPackument to Manifest its an extremely better name

whatcha think?

@soldair
Copy link
Contributor Author

soldair commented Aug 31, 2018

ok. i intend to change AbbreviatedPackument to Manifest 😉

@chrisdickinson chrisdickinson merged commit b025603 into npm:master Aug 31, 2018
@chrisdickinson
Copy link
Contributor

Thank you so much for this! Publishing as @npm/types @ 1.0.0.

@soldair
Copy link
Contributor Author

soldair commented Aug 31, 2018

curl https://registry.npmjs.org/@npm%2ftypes
{"error":"Not found"}

maybe went private?

@chrisdickinson
Copy link
Contributor

Ack! Good catch. I have made the package public (at 1.0.1 because reasons ✨)

@styfle
Copy link
Contributor

styfle commented Aug 31, 2018

Any reason why these types are not shipped with pacote or npm?

@zkat
Copy link

zkat commented Aug 31, 2018

Because we don't do typescript ourselves and I wasn't about to go through the trouble? I'll take a PR, though :)

@styfle
Copy link
Contributor

styfle commented Aug 31, 2018

@zkat It's actually quite easy to ship your .d.ts files alongside your .js source. All you need to do is make sure the pacote.d.ts file gets published (as you would expect) and add a single line to your package.json file such as "types": "./pacote.d.ts".

Now if you want to test the .d.ts file then you will need to add typescript to your devDependencies (as seen in this PR).

But shipping them together would prevent it from getting out of sync and also give end-users a MUCH better experience because they do import * as npm from 'npm' and it "just works" without any other packages.

@soldair
Copy link
Contributor Author

soldair commented Aug 31, 2018 via email

@zkat
Copy link

zkat commented Aug 31, 2018

@styfle it is non zero amounts of work for me in a storm of priorities. It doesn't matter if it's easy. If it's easy, all the more reason to get a PR from someone

@github-actions github-actions bot mentioned this pull request May 6, 2024
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.

5 participants