Upgraded to React 18, changed JsxParser (Fixes JsxParser is not a valid Jsx Element) #256
Conversation
There was a problem hiding this comment.
I left some comments - sorry for the slow response. I'm happy to get this merged once updated.
I do not see any benefit in moving to a functional component, and lots of downside. I will not approve a PR that does that. That said, I am perfectly happy to have a separately named functional component as a wrapper which renders the class component that's already here. I'm not sure if that provides a benefit, however.
Also, Circle CI is not passing with this build. Not sure why at the moment - I assume the build works for you?
| @@ -264,7 +273,7 @@ export default class JsxParser extends React.Component<TProps> { | |||
|
|
|||
| let children | |||
| const component = element.type === 'JSXElement' | |||
| ? resolvePath(components, name) | |||
| ? resolvePath(this.props.components, name) | |||
There was a problem hiding this comment.
This change shouldn't be needed, as components is already restructured above
| @@ -30,6 +30,15 @@ export type TProps = { | |||
| } | |||
| type Scope = Record<string, any> | |||
|
|
|||
| interface ForwardRefComponentProps { | |||
There was a problem hiding this comment.
This doesn't seem terribly useful as a type. Probably better just to use any or unknown here instead of essentially just defining a Record
| [key: string]: any; // for the rest of the properties | ||
| } | ||
|
|
||
| const ForwardRefComponent = React.forwardRef<any, ForwardRefComponentProps>((properties, ref) => { |
There was a problem hiding this comment.
Small thing, but I'd love to have this in a separate file, and call it something like just ForwardRef
| const className = [...new Set(['jsx-parser', ...String(this.props.className).split(' ')])] | ||
| .filter(Boolean) | ||
| .join(' ') | ||
| const className = [...new Set(['jsx-parser', ...String(this.props.className) | ||
| .split(' ')])].filter(Boolean).join(' ') |
There was a problem hiding this comment.
This line is already lamentably hard to read. I'd prefer we either make it readable or leave it alone. Breaking the [ ] onto separate lines makes it even more unclear, IMHO.
| ? ( | ||
| <div className={className}>{ | ||
| React.createElement(React.Fragment, {}, this.ParsedChildren) | ||
| } | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
I don't see any benefit to this change. It introduces a fragment where one isn't needed, and uses create element where it also isn't needed.
| @@ -1,18 +1,22 @@ | |||
| /* eslint-disable no-console */ | |||
| import React from 'react' | |||
| import ReactDOM from 'react-dom' | |||
| import { createRoot } from 'react-dom/client' | |||
There was a problem hiding this comment.
This is the only substantial change here, to support the latest rendering method. Could we please revert the rest? The demo script is designed to go with it's host html - I don't see much reason to safety check whether that hard coded page has an element on it. Also, this change appears to have altered the indentation and formatting quite a bit
|
Nice that this is so close to happen! |
|
I just pushed the necessary changes that @TroyAlford suggested. Please feel free to check them and consider merging the PR. |
|
@elitebyte , you have a PR ready to have the suggestions integrated. Feel free to ping me to discuss anything. |
|
@frandevel @elitebyte unfortunately, I'm trying to get tests to pass locally with a minimal subset of these changes, but something is wrong with the function parsing algorithms. Getting React 18+ working seems fine, and requires very little of what's been changed here, but I don't want to inadvertently create a regression with other features. |
|
I am closing this PR as dead. I have pushed some changes to the |
Strictly upgraded a few modules in order to get it working with React 18 including an enzyme alternative for React 18.
Also re-built so you can install off of my fork until PR is approved.
In package.json: