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 for domToReact #100

Merged
merged 8 commits into from Apr 5, 2019

Conversation

Projects
None yet
3 participants
@AndrewLeedham
Copy link
Contributor

commented Apr 1, 2019

What is the motivation for this pull request?

The docs use html-react-parser/lib/domToReact, but TypeScript definitions only exist for the core api (html-react-parser). I would say this is a feature.

Features

  • TypeScript definitions exist for html-react-parser/lib/domToReact.
  • Export domToReact and htmlToDom. (Fixes #84)

Checklist:

  • Tests
  • Documentation (includes TSDoc comments)
@coveralls

This comment has been minimized.

Copy link

commented Apr 1, 2019

Coverage Status

Coverage increased (+0.01%) to 99.296% when pulling 306fceb on AndrewLeedham:master into f4d4588 on remarkablemark:master.

@remarkablemark
Copy link
Owner

left a comment

Dependency html-dom-parser can now be updated to 0.2.0 so lint can pass

@AndrewLeedham

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Dependency html-dom-parser can now be updated to 0.2.0 so lint can pass

Hi @remarkablemark. I have been doing some testing with this today. It seems that the types folder structure only works for a single index.d.ts file. Therefore types/lib/dom-to-react is not recognised.

The possible solutions:

  1. Move the .d.ts files inline with the source code, so move dom-to-react.d.ts to <root>/lib/ and index.d.ts to <root>/.
  2. Provide domToReact as an export from index.js.

What are your thoughts on these options? Personally I think option 1 is better, as it doesn't change the current API, and allows for other files to be exposed in TS even if they aren't necessarily part of the public facing API.

An example of what option 1 would look like: remarkablemark/html-dom-parser#12.

@remarkablemark

This comment has been minimized.

Copy link
Owner

commented Apr 3, 2019

@AndrewLeedham Thanks for looking into this and coming up with both approaches.

I'm fine with either approach and I agree that the first is cleaner. But I also do think that it may be useful to expose domToReact as an export on index.js (see #84).

Given that, would you still like me to go ahead with merging remarkablemark/html-dom-parser#12?

@AndrewLeedham

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

@remarkablemark No problem.

Perhaps we should go with a combination of the two then. Have inline .d.ts files and export domToReact from index.js.

Either way I think the html-dom-parser changes are required, so merging that would make sense.

@remarkablemark

This comment has been minimized.

Copy link
Owner

commented Apr 3, 2019

Sounds good @AndrewLeedham. html-dom-parser@0.2.1 has been published

@remarkablemark remarkablemark merged commit c988f75 into remarkablemark:master Apr 5, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 99.296%
Details
@remarkablemark

This comment has been minimized.

Copy link
Owner

commented Apr 5, 2019

Thanks @AndrewLeedham for your effort and for making this possible

0.7.0 has been published

dlazarevexceedteam pushed a commit to dlazarevexceedteam/html-react-parser that referenced this pull request Apr 5, 2019

Merge pull request remarkablemark#100 from AndrewLeedham/master
Add TypeScript definitions for domToReact
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.