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

Changed passing children by props to nest between tags in docs #754

Closed
wants to merge 3 commits into from

Conversation

goofynugtz
Copy link

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Some of the examples mentioned in the readme are passing children as props which gives a ESLint error of react/no-children-prop on Next.js 13 with ESLint configured

<ReactMarkdown children={markdown}  />
yarn run v1.22.19
$ next build

Failed to compile.

./pages/blogs/[slug].tsx
72:13  Error: Do not pass children as props. Instead, nest children between the opening and closing tags.  react/no-children-prop

info  - Need to disable some ESLint rules? Learn more here: https://nextjs.org/docs/basic-features/eslint#disabling-rules
error Command failed with exit code 1.

Updated the examples to nest child component between the opening and closing tags.

<ReactMarkdown>{markdown}</ReactMarkdown>

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jul 14, 2023
Copy link
Member

@remcohaszing remcohaszing left a comment

Choose a reason for hiding this comment

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

Thanks! I agree passing children as actual children instead of a prop is better. I just have some nits about spacing.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (0242d11) 100.00% compared to head (46914bc) 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #754   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          754       754           
=========================================
  Hits           754       754           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@goofynugtz
Copy link
Author

Thanks for the reviews. I have made the suggested changes.

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

I'm cautious about this.
Yes as a general react practice, using nesting over the children prop is ideal.
That said, JSX spacing rules and template string spacing rules inside JSX are a frequent source of confusion for new adopters https://github.com/remarkjs/react-markdown/issues?q=is%3Aissue+JSX+is%3Aclosed

I'd lean against making this change.
I think it will cause more confusion than it clarifies.
But will not block the PR if other maintainers feel strongly otherwise.

@remcohaszing
Copy link
Member

If we don’t want this, I think we should rename the prop to something else, e.g. content or markdown. (A semver major is upcoming anyway.)

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jul 14, 2023

If we don’t want this, I think we should rename the prop to something else, e.g. content or markdown. (A semver major is upcoming anyway.)

I don't think it necessitates a major rework of the API.
There is a lot of overlap in adopters who don't understand that the two forms of children in react are equivalent and those don't understand the rules of JSX, and vice versa.

We could leave the docs as-is and let adopters choose which syntax they prefer.


To pre-emptively answer the question: "could we document the rules of JSX next to the example?"
Yes we could, but it wouldn't help much, no matter how many ":warning: IMPORTANT READ THIS!" we put around the example, many newcomers will ignore it, copy the example, and open an issue asking why it doesn't work as they'd expect (having not read the JSX spacing rules).

@wooorm
Copy link
Member

wooorm commented Jul 15, 2023

Did y’all see #749 (comment)?

I think people are going to try to pass children whether it works or not. As “text” inside the component, or as a sole expression inside the component. The main similar project to this, markdown-to-jsx, uses children too. And so do several other similar projects. Previously we used a different field, but a couple majors back, we moved to children, because it’s a common expectation from users.

Theoretically, we could throw on that. Or we could ignore it and support another field. I don’t prefer those over the current, sole, children.

The differences between <ReactMarkdown children={x} />, <ReactMarkdown>{x}</ReactMarkdown>, and <ReactMarkdown># literal X</ReactMarkdown>, is a bit complex for (somewhat) newcomers to React. But I feel that’s more a React fact. That it’s their responsibility. Which is fine to document with an example appendix and improved examples as in GH-749.


I also mostly agree with Christian! (only point that I think we can use one style in the examples, so people copy the nicest one, where currently 2 are shown)

I also think the ESLint rule is wrong: it’s good to use actual things in actual children normally, sure, but passing as a prop is useful in legitimate cases.

@wooorm
Copy link
Member

wooorm commented Sep 27, 2023

Used one style in the docs: <Markdown>{markdown}</Markdown>, so that solves this!

@wooorm wooorm closed this Sep 27, 2023
@wooorm wooorm added 📚 area/docs This affects documentation 💪 phase/solved Post is done labels Sep 27, 2023
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants