Skip to content

reactStringReplace should take/return ReactNodeArray #44

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

Merged
merged 1 commit into from
Apr 22, 2019

Conversation

monsterkrampe
Copy link
Contributor

fixes #43

@monsterkrampe
Copy link
Contributor Author

monsterkrampe commented Apr 19, 2019

I commented my thoughts at the specific lines.

I would be happy to discuss this with you @iansinnott :)

Also any feedback from others seeing this PR is very welcome, since I would not consider myself a typescript expert.

@monsterkrampe monsterkrampe marked this pull request as ready for review April 19, 2019 09:42
@iansinnott
Copy link
Owner

Are React.ReactNodeArray and React.ReactNode built in to typescript? Where are they defined?

@monsterkrampe
Copy link
Contributor Author

These are defined in the react type definitions just like JSX.Element which we had before.

I would suggest adding react as a peerDependency anyway, but this depends on how you intend to use react-string-replace. Just seeing the code, one could assume that this module can be used for anything, not just react.
If this is your intention, I would not like to use React.ReactNode and so on and I would also not add react as a peerDependency.
But since I think that it is indeed intended to use this module only in combination with react, I think react should be added as a peerDependency, although I'm not sure which version.
(hope this is not too off topic)

Let me know what you think.

@iansinnott
Copy link
Owner

I think that it is indeed intended to use this module only in combination with react

Yeah exactly, even though the code isn't React-specific in practice I would think everyone's using it with React. So I'm happy with the React-centric type definitions.

Thanks for the clarification about where the definitions come from.

@iansinnott iansinnott merged commit 87b61e2 into iansinnott:master Apr 22, 2019
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.

Typescript definitions are wrong (I think)
2 participants