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

Adds whether there are types for a package to an index #346

Merged
merged 4 commits into from
Jul 1, 2019

Conversation

orta
Copy link
Contributor

@orta orta commented Jun 14, 2019

Based on this discussion. This PR adds a new field to the JSON index for npm search

{
  ...
  "types": { "ts": "included" }
}

or

{
  ...
  "types": { "ts": "@types/lodash" }
}

and if they don't support it:

{
  ...
 "types": {  "ts": null }
}

which might be absent in JSON? Or false. Actually not sure. But definitely falsy.

Running yarn format did a few more changes than I would have done solo, but I'll leave comments in here about what I've changed.

Note: I've not been able to successfully run this locally against a set of packages, so right now it's somewhat ready for testing but not guaranteed to be shippable without risk. Should work tho - hah.

Adding comments now.

README.md Show resolved Hide resolved
src/formatPkg.js Outdated Show resolved Hide resolved
src/npm.js Show resolved Hide resolved
src/saveDocs.js Show resolved Hide resolved
src/typescriptSupport.js Show resolved Hide resolved
@Haroenv
Copy link
Collaborator

Haroenv commented Jun 14, 2019

@mjackson, are you fine with us calling unpkg here this many times?

src/typescriptSupport.js Outdated Show resolved Hide resolved
@Haroenv
Copy link
Collaborator

Haroenv commented Jun 14, 2019

Would be cool if there were tests for the new code :) looking at it, i think we’ll be fine with the impact on indexing time

@orta
Copy link
Contributor Author

orta commented Jun 14, 2019

It would do a head request to unpkg at most once per package given that it's likely the majority of packages don't have types, then I'd expect the majority of packages to do this

@Haroenv
Copy link
Collaborator

Haroenv commented Jun 15, 2019

I think we should nest this inside a “types” property, so that if there’s interest, we can add flow & reason eg

@orta
Copy link
Contributor Author

orta commented Jun 15, 2019

Makes sense - it does that now, and has tests 👍

@robb
Copy link

robb commented Jun 15, 2019

null might be a more convenient alternative to undefined, given that the latter isn't available in JSON…

@orta
Copy link
Contributor Author

orta commented Jun 15, 2019

Yeah, great point - fixed!

@arcanis
Copy link

arcanis commented Jun 17, 2019

The only caveat is that the index for foo won't be recomputed when a new @types/foo package appears, am I correct? It will require to publish a new version of foo 🤔

@Haroenv
Copy link
Collaborator

Haroenv commented Jun 17, 2019

There’s a weekly refresh of the whole index to catch things like this. I guess we could add some extra code to schedule an update of the relevant package of an @types but it’s not that big of an issue I think

@mjackson
Copy link

@Haroenv I'm 👍 with calling unpkg like this. I'll let y'all know as soon as I launch a better way to do this than making HEAD requests...

@Haroenv
Copy link
Collaborator

Haroenv commented Jun 29, 2019

Perfect 👌 I’ll take a look at if this works well next week

README.md Outdated Show resolved Hide resolved
src/__tests__/npm.test.js Outdated Show resolved Hide resolved
orta and others added 2 commits July 1, 2019 10:58
Co-Authored-By: Haroen Viaene <fingebimus@me.com>
Co-Authored-By: Haroen Viaene <fingebimus@me.com>
@Haroenv
Copy link
Collaborator

Haroenv commented Jul 1, 2019

@orta I don't seem to be able to commit my comments, can you do it or give access to the repo? I'd like to try it out live

@Haroenv
Copy link
Collaborator

Haroenv commented Jul 1, 2019

hah, faster than my comment

@orta
Copy link
Contributor Author

orta commented Jul 1, 2019

👍

@Haroenv Haroenv merged commit d32f978 into algolia:master Jul 1, 2019
@karlhorky
Copy link

Awesome, merged! 🙌

I guess the next step to get this online is for someone to do a PR to yarnpkg/website as per
@arcanis' tweet?

And then something for the npm website too?

cc @mxstbr

@Haroenv
Copy link
Collaborator

Haroenv commented Jul 8, 2019

Yes, the next step would be to add this to the website since it's now fully indexed

@Haroenv
Copy link
Collaborator

Haroenv commented Jul 8, 2019

https://github.com/yarnpkg/website/blob/master/js/src/lib/Details/Aside.js here probably

@Haroenv
Copy link
Collaborator

Haroenv commented Jul 8, 2019

Looks like something went wrong here still, I'll investigate, but the whole index has ts: null as the types key

@orta
Copy link
Contributor Author

orta commented Jul 8, 2019

Doh!

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