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 typescript definitions #351

Merged
merged 5 commits into from
Jul 19, 2017
Merged

add typescript definitions #351

merged 5 commits into from
Jul 19, 2017

Conversation

giladgray
Copy link

Fixes #345

  • add index.d.ts to popper package (and types entry in package.json)
  • add a note in PULL_REQUEST_TEMPLATE to remind folks to keep this updated

@CLAassistant
Copy link

CLAassistant commented Jul 12, 2017

CLA assistant check
All committers have signed the CLA.

@giladgray
Copy link
Author

@FezVrasta is there any work required to ensure the new index.d.ts file gets published to NPM? also, are you okay with placing it in the package root?

@giladgray
Copy link
Author

@FezVrasta wow that CLA-assistant wants a lot of permissions! Why does it need to write to my public repos?

@@ -1,9 +1,11 @@
<!--
Thanks for your interest in contributing to Popper.js!
Copy link
Member

Choose a reason for hiding this comment

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

Please don't remove the trailing whitespaces, they are meaningful in markdown

Copy link
Author

Choose a reason for hiding this comment

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

my editor did that. but also this is a comment, so they're really not meaningful here: it is invisible in rendered output.

i much prefer using blank lines to create paragraphs in markdown (instead of the awkward trailing two spaces) because i always have my editor set to trim trailing whitespace.

Copy link
Author

Choose a reason for hiding this comment

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

also blank lines are visible in source so i can tell immediately that you meant to have two paragraphs.


### Make sure to

1. Make sure the tests are passing, you can check it running `npm test` with Google Chrome installed;
2. Add any relevant test to cover the code you have changed and/or added;
3. If you change the public API, update the Typescript definitions accordingly:
Copy link
Member

Choose a reason for hiding this comment

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

Didn't you decide to maintain them yourself? If this is still the case is rephrase this to a "nice to have but not requiree action"

Copy link
Author

Choose a reason for hiding this comment

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

i'll take responsibility for this file and be the first line of defense should something go wrong, but i think it's worth adding a comment like this so folks know it exists.

if someone forgets, or has trouble editing it, ping me for help. i'll amend the note to this effect.

Copy link
Member

@FezVrasta FezVrasta Jul 13, 2017

Choose a reason for hiding this comment

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

As stated previously, I don't want to lose contributions because someone gets scared reading he needs to know TypeScript to contribute.

If the point says "If you change the public API, it would be nice to try to update the TypeScript definitions accordingly. This is not required but will help a lot." (or something similar) then I'd be happy to merge this.

Otherwise, feel free to contribute to the definitely typed project, doing so you can keep the definitions up to date and be completely independent.

Copy link
Author

Choose a reason for hiding this comment

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

I'd be wary of contributors changing the public API at will--probably not likely to happen often, but when it does then special attention will be essential anyway.

i'll update the message to be more accomodating

@giladgray
Copy link
Author

@FezVrasta added a note about myself in PR template. let me know what else you'd like to see here. would be great to merge and ship a new release soon so I can consume it in Blueprint!

@FezVrasta
Copy link
Member

FezVrasta commented Jul 14, 2017

ok it looks great now. I'm not sure how this new file you added gets read by TypeScript but:

  1. Right now it will not be included in the npm package because we include only dist/*
  2. If people import from dist/esm/popper.js should the new file be in that folder as well with the same name?
  3. Same of point 2 applies for the umd build and the next/es build on the dist/ root directory

@giladgray
Copy link
Author

The types entry in package.json tells TS compiler where to find typings, that's the only thing that matters.
Fair to add index.d.ts to files list then?

@FezVrasta
Copy link
Member

Yes sure

@FezVrasta
Copy link
Member

released as 1.11.0

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.

3 participants