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 definitions of ajv #10498

Merged
merged 1 commit into from
Aug 8, 2016
Merged

add definitions of ajv #10498

merged 1 commit into from
Aug 8, 2016

Conversation

plantain-00
Copy link
Contributor

@plantain-00 plantain-00 commented Aug 7, 2016

case 1. Add a new type definition.

  • checked compilation succeeds with --target es6 and --noImplicitAny options.
  • has correct naming convention
  • has a test file with the suffix of -tests.ts or -tests.tsx.

@dt-bot
Copy link
Member

dt-bot commented Aug 7, 2016

ajv/ajv.d.ts

Checklist

@sandersn sandersn merged commit dd0d729 into DefinitelyTyped:master Aug 8, 2016
@blakeembrey
Copy link
Member

blakeembrey commented Nov 30, 2016

@sandersn It doesn't look like this should have been merged, the type definitions were already contributed to the library in April - see ajv-validator/ajv#163. Is there any way you can make checking the library part of the PR process so these get rejected - it's not the first one I've come across now, since people misunderstand how it all works and there's a lot of issues like ajv-validator/ajv#225 (not everyone asks the library author first).

@sandersn
Copy link
Contributor

At the very least we should check notNeededPackages.json and give an error if it's in there. @andy-ms does this already happen? Our bundling instructions tell authors to update notNeededPackages.json. (Although, note that file didn't exist in August.)

However, I don't think checking the library itself would be reliable enough since it depends on authors following the bundling instructions correctly. @andy-ms @RyanCavanaugh do you have thoughts on this?

On the human side, we could also:

  1. Add "please check the library for existing typings" to the PR template check list.
  2. Add the same request to the 'create a new package' instructions.

@blakeembrey
Copy link
Member

Sounds great 👍 Though the manual list might be tricky for authors, I definitely don't add every library I publish to that list so I wouldn't expect others to. Seems like it's more useful for deprecation from DefinitelyTyped as you can't automate it for new packages (side note: would it help to have a job that updates your list with packages found on NPM with types fields automatically?).

Why wouldn't checking the library be reliable enough? If it can't be checked automatically, there's no way TypeScript would resolve it. At the very least, it seems like it'd be a simple automatic check which compliments the other points. Sure, there's still some cases where it won't work (e.g. the author uses typings.json), but it'd be a good start and Typings will (hopefully) be able to go away soon enough.

@plantain-00
Copy link
Contributor Author

FYI: I started a PR to remove the types. #13028

@ghost
Copy link

ghost commented Dec 1, 2016

notNeededPackages.json is just for libraries deprecated from @types, not for every library in existence.

We already do include The package does not provide its own types, and you can not add them. in the PR request template. There is similarly a warning on the first line of the "create a new package" instructions.

I was thinking of adding a check in types-publisher that warns for any packages that have added their own typings. We could do something similar in dt-bot too.

@blakeembrey
Copy link
Member

blakeembrey commented Dec 1, 2016

Thanks @andy-ms! Having it automated would be a nice-to-have as I've seen not everyone is actually aware that modules can publish the .d.ts files and instead think that you must publish using NPM @types. It's resulted in some fun conversations such as pillarjs/router#48 - which, to note, might be some place that TypeScript can improve documentation.

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.

4 participants