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 flag for TypeScript modules #126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blakeembrey
Copy link

@blakeembrey blakeembrey commented Feb 2, 2017

Guidance on a feature like this would be extremely useful. Let me know if anything is missing for this functionality. I am hoping to enable a flag like is:typescript (or is:types?) so people can find NPM packages that publish TypeScript definitions (TypeScript users can have trouble finding modules they can use currently, this feature would show TypeScript users the packages that publish with a type definition).

Edit: Also, is there a way to populate the fixtures programmatically so I can enable a test case?

@coveralls
Copy link

coveralls commented Feb 3, 2017

Coverage Status

Coverage increased (+0.004%) to 97.41% when pulling 0c5865aaa367d3fa1ae3bd3c8314b85429982b23 on blakeembrey:types-flag into 79279de on npms-io:master.

@coveralls
Copy link

coveralls commented Feb 3, 2017

Coverage Status

Coverage increased (+0.004%) to 97.41% when pulling 0c5865aaa367d3fa1ae3bd3c8314b85429982b23 on blakeembrey:types-flag into 79279de on npms-io:master.

@atduarte
Copy link
Member

atduarte commented Feb 3, 2017

🎉 So awesome! 🏆 Great idea and great initiative! I will look into more detail tomorrow (it's already 1AM in Portugal 😄).

In the meantime, I'm not entirely sure about the "is:typescript", because semantically it means that it's written in TypeScript but the flag just means that it's TypeScript compatible.

Relevant: https://github.com/npms-io/queries/blob/f2f6e2ee9c11a476f62ccce0e057c2412ca3050e/lib/util/parseSearchQuery.js#L36

@satazor look at this! 😃

@blakeembrey
Copy link
Author

blakeembrey commented Feb 3, 2017

@atduarte Absolutely, that's understandable. Thanks for responding! I was worried no one would after I logged the first issue as a suggestion 😄

I stuck with the is: style since that seemed to be the "boolean" style of querying. Of course, it could be anything else. Also true, it doesn't mean it's written in TypeScript but rather TypeScript-compatible. Do you have thoughts on a better flag? I was also considering is:types, but didn't want to cause any tension with Flow (although, TypeScript already uses the @types namespace on NPM, uses types in package.json and the Github org @types is for TypeScript definitions - so maybe that's a non-issue?).

Edit: Maybe has: as a boolean prefix (basically matching is: anyway, but more semantically meaningful). That way it'd be has:types?

@satazor satazor closed this Mar 26, 2017
@blakeembrey
Copy link
Author

Could you let me know if there was an issue with the PR?

@satazor
Copy link
Member

satazor commented Mar 26, 2017

Sorry I closed this by mistake, I multi-selected several greenkeeper related issues and selected this by mistake!

@satazor satazor reopened this Mar 26, 2017
@coveralls
Copy link

coveralls commented Mar 26, 2017

Coverage Status

Coverage decreased (-0.6%) to 96.802% when pulling 0c5865aaa367d3fa1ae3bd3c8314b85429982b23 on blakeembrey:types-flag into 79279de on npms-io:master.

@@ -34,6 +34,7 @@ function inspectFiles(data, dir) {
testsSize: detectRepoTestFiles(dir).then((files) => fileSize(files)),
hasNpmIgnore: isRegularFile(`${dir}/.npmignore`).then((is) => is || null),
hasShrinkwrap: isRegularFile(`${dir}/npm-shrinkwrap.json`).then((is) => is || null),
hasTypes: isRegularFile(path.join(dir, data.typings || data.types || 'index.d.ts')).then((is) => is || null),
Copy link
Member

Choose a reason for hiding this comment

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

Everything looks great, except the hasTypes which is too generic and doesn't resemble typescript. Can you change to something more explicit?

Copy link
Author

Choose a reason for hiding this comment

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

No problem. How about naming it hasTypeScript instead?

@blakeembrey
Copy link
Author

@satazor I updated it to hasTypeScript. I also noticed that it's possible to cause an error to be through here with malformed data.typings. Should I just use String(data.typings || data.types || 'index.d.ts') here instead or do you have another approach in use?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 96.812% when pulling 769f56472839ea0b9fac35b3fc6aeb93e444e5e6 on blakeembrey:types-flag into 69cade6 on npms-io:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 96.812% when pulling 769f56472839ea0b9fac35b3fc6aeb93e444e5e6 on blakeembrey:types-flag into 69cade6 on npms-io:master.

@satazor
Copy link
Member

satazor commented Apr 2, 2017

@blakeembrey Yes I would convert it to a string just to be sure otherwise path will throw right? Other fields are guaranteed to be correct because we use the normalize-package-data module that does that for npm known fields. Anyway, I think you mean packageJson.types|typings instead of data.types|typings. The data is the whole couchdb document of the package.

Regarding the isTypeScript, etc. I agree with @atduarte: we should make it clear that the package has ts typings instead of assuming it's typescript. There are a lot of packages that are not written in ts but offer typings. What about hasTypeScript -> hasTypeScriptTypes, is:typescript has:typescript-types? Too verbose?

@blakeembrey
Copy link
Author

@satazor Yes, I can do that. In that case, should I change data to packageJson and update the function signature of inspectFiles to accept packageJson?

For "is typescript", I definitely understand the want to have a better distinction. Since the query is case-insensitive and likely typed often, something simple would be nice. That was why I went with the simple solutions of is:typescript or (is|has):types. I can live with has:typescript-types, it just feels heavy to write out. I can update the PR once we have a good solution, or use typescript-types for now and alias it later. Maybe even has:dts or is:dts (after the .d.ts extension used for TypeScript definitions)?

@satazor
Copy link
Member

satazor commented Apr 2, 2017

@blakeembrey Yes you can!

I like the idea of adding typescript-types and a d.ts alias. What does the d means there (never used typescript)?

@blakeembrey
Copy link
Author

.d.ts is just the extension used when writing a "declaration" file manually or generated by TypeScript when using the declaration option (e.g. https://github.com/blakeembrey/node-htmlmetaparser/blob/15a206ac8bc70e4cdfdcf22b268b3e70013bb58d/tsconfig.json#L9). It contains all the type information TypeScript uses, while the .js file would contain the "runtime".

Is there an existing approach to aliases? I went ahead with the dts naming, but if there's a preferred way for it to be written feel free to let me know (or just commit it yourself, I don't want to hold you up since it's probably a bunch of trivial changes around the place).

@coveralls
Copy link

coveralls commented Apr 3, 2017

Coverage Status

Coverage increased (+0.005%) to 96.812% when pulling 40d1027 on blakeembrey:types-flag into e81892b on npms-io:master.

@caseyWebb
Copy link

Hopefully this isn't dead, as it would be a great feature to have; since migrating to TS I've found that finding high-quality typed packages to be one of the most difficult, or at least time consuming, aspects.

Syntactically, what about something like types:ts? This would set aside a namespace should Flow (or any other JS type system) support be desired in the future (i.e. types:ts, types:flow, etc...).

Also, it'd nice if during the analysis (or whenever), it also checked for a DefinitelyTyped repo under the same name, (e.g. jquery isn't authored in TS or come with bundled definitions, but they exist at @types/jquery). That said, some sort of distinction would be nice since bundled definitions are generally of higher quality.

@orta
Copy link

orta commented Oct 18, 2019

For the Yarn search powered by Algolia's npm-search index we ended up with this object shape:

Not included:

{
  types: { ts:false }
}

In definitely typed:

{
  types: { 
     ts: 'definitely-typed',
     definitelyTyped: '@types/my-lib',
   }
}

Embedded inside the dependency

{
  types: { ts: 'included' }
}

Basically:

type TypeAvailability = 
   | { ts: false }
   | { ts: "definitely-typed",  definitelyTyped: string } 
   | { ts: "included" }

interface Result {
  ...
  types: TypeAvailability
}

This allows tools like the TypeScript playground to make type acquisition with a lot less networking requests, as well as the option to one day add flow support.

Maybe having the same shape solves the discussion and lets it be consistent?

I know the NPM folks have been interested in getting this feature on the npm site, so it might be worth trying to get this back alive.

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