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

drop dependency on react 15 #107

Closed
talbet opened this issue May 29, 2019 · 13 comments

Comments

Projects
None yet
3 participants
@talbet
Copy link

commented May 29, 2019

There have been a few discussions about problems with bundling react 15 and react 16 side by side (#101 #105). Since react-dom-core seems to only be used for creating look-up tables for html attributes, and since react15 hasn't seen an update in two years, would it make sense to extract these lookups into html-react-parser and drop the react-dom-core dependency?

This may require dropping support for react 15 so I understand if this is not a good direction.

@remarkablemark

This comment has been minimized.

Copy link
Owner

commented May 30, 2019

Thanks for opening this issue @talbet

You're correct in that html-react-parser only relies on react-dom-core for the HTML and SVG property configs.

In the past, I considered keeping a map of the attributes in this package, but I ended up deciding against due to the additional scope of having to update it manually if the attribute map ever changes.

However, like you mentioned, it seems that react 15 will see little or no updates in the foreseeable future so I'm willing to look into this again.

If we are to go forth with stripping out the dependency, I'd like to accomplish the following:

  • keep the properties config files as a separate package (i.e., add it as a new package in the react-dom-core monorepo),
  • build the attribute map from react 15,
  • allow the map to be modified before publish,
  • ensure no breaking changes when used in html-react-parser

What are your thoughts?

@talbet

This comment has been minimized.

Copy link
Author

commented May 31, 2019

Those all sound sensible. If you want I can try and get a PR together for react-dom-core some time next week?

@remarkablemark

This comment has been minimized.

Copy link
Owner

commented Jun 9, 2019

Hey sorry for the late reply, yes I'm open to that. Let's discuss what I can do to help.

@talbet

This comment has been minimized.

Copy link
Author

commented Jun 9, 2019

I'm on vacation for the next two weeks. Was hoping to get something together before then, but got caught up on other things. Once I get back we can have a chat and work out how to proceed.

@remarkablemark

This comment has been minimized.

Copy link
Owner

commented Jun 10, 2019

No worries! Please do enjoy your time off @talbet. Let's sync up once you're back.

@Amin52J

This comment has been minimized.

Copy link

commented Jun 12, 2019

This is not a feature, this is a bug breaking apps because it's loading react v15 next to the newer versions of react.

@remarkablemark

This comment has been minimized.

Copy link
Owner

commented Jun 12, 2019

What version of html-react-parser are you using @Amin52J?

$ npm ls html-react-parser

The bug you're experiencing should be fixed in 0.7.1

@Amin52J

This comment has been minimized.

Copy link

commented Jun 12, 2019

v0.4.7

The thing is when html-react-parser is added as a dependency the next.js build is complaining about Invalid use of hooks pointing to a line using html-react-parser, but when it's removed it's still showing the same error pointing to somewhere else in the code, so I might be wrong that it's html-react-parser that's in fact breaking the build.

@Amin52J

This comment has been minimized.

Copy link

commented Jun 12, 2019

I confirm that the issue was not caused by html-react-parser.

@remarkablemark

This comment has been minimized.

Copy link
Owner

commented Jun 12, 2019

Thanks for confirming @Amin52J. Do let me know if you uncover anything new.

@talbet

This comment has been minimized.

Copy link
Author

commented Jun 28, 2019

It looks like react-dom is being used for 4 things:

  1. a function that can determine custom attributes like data- and aria-
  2. a check to see if an attribute is a boolean attribute
  3. a map for html attributes to react props
  4. a map for svg attributes to react props

I propose they could be handled like this:

  1. Probably easiest just to copy this function across
  2. Could be extracted to an array of strings since it's only 24 attributes
  3. About 150 key/value pairs. Could either:
    • extracted as an object, or
    • build at runtime with the array of valid react props, and then running toLowerCase() over them and merging the 4 exceptions (class -> className).
  4. About 400 key/value pairs. Could either
    • extracted as an object, or
    • build at runtime with the array of valid svg attributes, and then converting kebab case attributes to camel case props.

Let me know what you think @remarkablemark and if I've missed anything?

@remarkablemark

This comment has been minimized.

Copy link
Owner

commented Jul 3, 2019

I think that covers all bases. Thanks for thinking this through @talbet!

I ended up building something really quick and I'd love to get your feedback. We can use it as an an initial foundation to build on top of.

See remarkablemark/react-dom-core#9

remarkablemark added a commit that referenced this issue Jul 8, 2019

feat(attributes-to-props): replace `react-dom` with `react-property`
Save `react-property@0.1.0` to package.json dependencies and remove
`react-dom-core`.

Delete `lib/property-config.js` given that the mapping is already
created in `react-property`.

Fix incorrect test since both `ychannelselector` and
`yChannelSelector` are applicable SVG attribute names.

Resolves #107
@remarkablemark

This comment has been minimized.

Copy link
Owner

commented Jul 9, 2019

@talbet I released and published html-react-parser@0.9.0:

# npm
npm i -S html-react-parser@0.9.0

# yarn
yarn add html-react-parser@0.9.0
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.