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

TypeScript Support #116

Closed
wants to merge 6 commits into from
Closed

TypeScript Support #116

wants to merge 6 commits into from

Conversation

bencompton
Copy link

I am using React Portal in a TypeScript project and I ended up creating these typings. I tested these changes by cloning this repo, applying the changes in this pull request, then referencing the repo from my project via a local NPM file reference in my project's package.json.

For anyone not familiar with TypeScript, the TypeScript docs suggest that the best option is to have the typings exist in the original repo of a project so they can be more easily maintained part of the project itself as API changes occur. The typings in my PR should reflect the React Portal API as it exists now, but note that if this PR is accepted, then that will mean that all future public API changes will require updating these typings as well.

The second choice option is to publish these typings to DefinitelyTyped and have them be maintained separately from the original project. So, I'm trying for the best option first.

@bencompton
Copy link
Author

It appears my React typings (@types/react) dependency is getting a 404 from Travis CI, but I just tried a NPM install and it does resolve for me. I'll investigate.

@bencompton
Copy link
Author

Ok, the Travis CI checks are now passing. It appears that the .travis.yml setting to cache the node_modules directory was causing the problem. Evidently, Travis CI doesn't have any logic to invalidate the node_modules cache when new dependencies are added in the package.json file. The solution was to manually invalidate the cache by removing the cache setting from .travis.yml, letting it build, and then adding the cache setting back.

@tajo
Copy link
Owner

tajo commented Dec 9, 2016

Is there some way to test that .ts file? Since I don't use TypeScript it would be good to have some sort test that would tell me if these definitions are in sync with the code.

@bencompton
Copy link
Author

Agreed, tests should be incredibly helpful for ongoing maintenance of this. From what I've seen, the typical way to test typings is to have .ts files that exercise the API, then invoke the TypeScript compiler from a test. Redux is an example of a library not written in TypeScript that has the typings published along with it. They have test TypeScript files like this that they test like this. In the case of a React component, it may also be a good idea to render it in Enzyme in addition to setting its props in TypeScript.

If that sounds good to you, I'll work on getting tests like this added to the PR.

@bencompton
Copy link
Author

Ok, I updated the PR with some tests.

For typings submitted to DefinitelyTyped, they only require creating a TypeScript file that exercises the API, and they then test that it compiles. Redux basically has the same thing. However, what that approach fails to do is to verify that the TypeScript definitions actually match the API defined in JavaScript, so it is possible for the APIs to get out of sync without the tests failing.

Therefore, in addition to the usual TypeScript static compilation test, I also created added couple more tests that use TypeScript to create an instance of a Portal to make sure it mounts successfully and also that the props match the propTypes defined in Portal.js.

To verify that these tests are working correctly, I changed prop names in portal.d.ts and exercise-all-props.ts without changing any JavaScript. As expected, the tests failed. Likewise, changing propTypes in the Portal JavaScript without updating the TypeScript definitions and tests will cause test failure.

@tajo
Copy link
Owner

tajo commented Dec 29, 2016

Can someone else using TypeScript take a look? We should probably also support flow.

Anyway, I'm going to wait with this untill all other v4 PRs are merged since some APIs might change.

Thanks!

@tajo tajo added this to the v4 milestone Dec 29, 2016
@Jontem
Copy link

Jontem commented Mar 2, 2017

We're looking at using react-portal in our typescript project. The typings for V3 is a little bit of and i found this PR which seems to address my issue. Is there something i can help out with?

When is v4 going to be released?

One thing that would help with typescript typings is if the library could also export Portal as a named export. It can coexist with the default export. The default export is problematic when using ES6 import style and transpiling to other module formats and is hard to create good typing declarations for.

Named exports works good for all module formats.

One solution that makes default exports work would be to use the module module field. So we can import es6 code without transpiling directly in our bundlers(E.g webpack). Then in the typings just do export default Portal

@tajo
Copy link
Owner

tajo commented Oct 1, 2017

Hey, please check the new major version (complete rewrite) of react-portal: #157

It's React v16 only since its uses the new official Portal API. There is the first beta released and I would like to get your feedback. I don't have bandwidth to maintain v3 which is very different and full of hacks.

If you feel your PR still applies, please rebase it against the master and re-open it. Sorry for the lack of response in past! Thanks!

@tajo tajo closed this Oct 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants