Skip to content

Add TypeScript definitions for domToReact#100

Merged
remarkablemark merged 8 commits intoremarkablemark:masterfrom
AndrewLeedham:master
Apr 5, 2019
Merged

Add TypeScript definitions for domToReact#100
remarkablemark merged 8 commits intoremarkablemark:masterfrom
AndrewLeedham:master

Conversation

@AndrewLeedham
Copy link
Copy Markdown
Contributor

@AndrewLeedham AndrewLeedham 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

Checklist:

  • Tests
  • Documentation (includes TSDoc comments)

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 1, 2019

Coverage Status

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

Copy link
Copy Markdown
Owner

@remarkablemark remarkablemark left a comment

Choose a reason for hiding this comment

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

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

@AndrewLeedham
Copy link
Copy Markdown
Contributor Author

AndrewLeedham 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
Copy link
Copy Markdown
Owner

@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
Copy link
Copy Markdown
Contributor Author

@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
Copy link
Copy Markdown
Owner

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

@remarkablemark remarkablemark merged commit c988f75 into remarkablemark:master Apr 5, 2019
@remarkablemark
Copy link
Copy Markdown
Owner

Thanks @AndrewLeedham for your effort and for making this possible

0.7.0 has been published

d-lazarev pushed a commit to d-lazarev/html-react-parser that referenced this pull request Apr 5, 2019
Add TypeScript definitions for domToReact
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