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

Proposal: Remove React.DOM (dom component factories) API and release as new module #6169

Closed
zpao opened this issue Mar 2, 2016 · 27 comments
Closed
Assignees
Milestone

Comments

@zpao
Copy link
Member

zpao commented Mar 2, 2016

The React.DOM API (not to be confused with ReactDOM) does not make sense in the greater scheme of the React API being used in other rendering environments like React Native or GL or <insert anything that isn't the DOM>. The React.* API is meant to be universal in that sense. We are never going to add React.iOS.

However, these factories don't actually have any implementation details that rely on actual DOM details, they essentially just map like so: React.DOM.div = (...args) => React.createElement('div', ...args), which is why they ended up in the universal React package and not in ReactDOM.

This would only impact people who don't currently use JSX and have held out using the function call syntax. <div/> now (and has for over a year) transforms to React.createElement('div'). The conveniences of the function syntax are a big deal for many people (especially coffeescript users and those who are strongly anti-JSX) so we don't want to break anybody, but we would like to migrate them from the core React API.

So the proposal would be a new package which just exports the exact set of things that React.DOM currently makes available. It would require some small changes to your code and a new package dependency, but is otherwise identical. Here's what the change might look like.

// before
{div, span} = React.DOM

// after
{div, span} = require('react-some-other-package');
@mxstbr
Copy link
Contributor

mxstbr commented Mar 2, 2016

👍

I discovered React.DOM.<element> recently, and was really confused as to why that was there. I think it's a good idea to have that in a separate package, though it would probably need a major version increase. (Depreciation with warnings maybe?)

@sophiebits
Copy link
Collaborator

These also work today:

const div = React.createFactory('div'); 
// then div(...)
const e = React.createElement;
// then e('span', ...)

@feifanzhou
Copy link

Would it make sense to add this as part of ReactDOM rather than adding to package fatigue with another module?

@zpao
Copy link
Member Author

zpao commented Mar 2, 2016

Potentially but we would have to change the API so there would be code changes required anyway (ReactDOM.DOM is bad). Personally I think it would make more sense in a standalone package. Ideally we'd like to move away from whitelists like this in React and ReactDOM itself. There's nothing stopping you from doing React.createElement('awesome-new-spec-element') but maintaining a whitelist requires a new release and it can be versioned independently and be used with different React versions. Adding React.DOM['awesome-new-spec-element'] would require a new release and would be truly obnoxious to add to every possible version so we'd likely only add it in the next minor version, not backporting. Whereas a separate package could get its own bump without having to pick up other React changes.

@matystl
Copy link

matystl commented Mar 2, 2016

👍
Also what would be nice to get inspiration from hyperscript-helpers especialy have richer interface for element functions https://github.com/ohanhi/hyperscript-helpers#api

@sebmarkbage
Copy link
Collaborator

Which ReactDOM? react-dom, react-dom/server or react-dom-third-party...? The point is to not have a dependency from a normal component into the specific renderer so that you can switch out the renderer.

@zpao
Copy link
Member Author

zpao commented Mar 3, 2016

@matystl I really don't think we'll change any APIs or behaviors. The idea is not to make these methods more flexible and support different use cases, simply to move them from their current location. People are welcome to use hyperscript or similar if they want other APIs instead.

@jquense
Copy link
Contributor

jquense commented Mar 6, 2016

is there somethng to PR here? would you just add another build step to produce the package from the main repo, or a new repo just for the factories (perhaps in the reactjs org)? In any case I'd be happy to PR something, just not sure what the best way to do it might be

@zpao
Copy link
Member Author

zpao commented Mar 7, 2016

I would probably move it to a separate project in the org, but nothing for anybody to do at the moment.

@chicoxyzzy
Copy link
Contributor

In future this could be implemented as easily as:

import { createFactory } from 'react'; // 'react-dom'? 'react/dom'?

export default new Proxy(Object.create(null), {
  get(target, type) {
    return createFactory(type);
  }
});

@syranide
Copy link
Contributor

@chicoxyzzy Not if you care about browser support or GCC advanced optimization.

@moodysalem
Copy link

Does this already do it?
https://www.npmjs.com/package/r-dom

@zpao
Copy link
Member Author

zpao commented Mar 25, 2016

Yes however that depends on the react package (which we wouldn't directly, would probably be a peerDep?) and it supports adding extra properties (eg isRendered) which we also wouldn't do.

@chicoxyzzy
Copy link
Contributor

about browser support

that's why I said "in future" :)

or GCC advanced optimization

this might be an issue but I think it could be solved

@tj
Copy link

tj commented Mar 26, 2016

Is there any reason to have these functions at all, vs JSX compiling to plain objects? It could be more user-friendly for the hand-written cases as well, and no need to import 'react' all the time or add it as a dep for many components.

@gaearon
Copy link
Collaborator

gaearon commented Mar 27, 2016

I don’t have full context on this so I can’t give a good reply. Here’s what I know about this:

  • We still have a concept of “owner” which is a dynamic thing set by createElement that relies on global state about the currently executing render() method. I think the plan is to get rid of it (Remove ReactCurrentOwner #6350) but that would mean deprecating string refs. Until _owner is gone we can’t generate objects all the time without screwing up string refs.
  • We added a certain level of protection against XSS in user input in Use a Symbol to tag every ReactElement #4832 which tags every “safe” element coming from the code (as opposed to a user JSON) with a Symbol. These would be a bit of a pain to write by hand.
  • There is a Babel plugin that makes JSX output inline objects. However we only recommend it in production because it effectively bypasses any propTypes validation or useful developer warnings.

@chicoxyzzy
Copy link
Contributor

Will fiber reconciler help to solve owner concept issue anyhow?
Could the issue with inline objects Babel plugin be solved by FlowType or TypeScript? I think it's a very non-trivial task. Also we can't force people to use static typing.

The whole issue looks too complicated. Do you mind to split it into different tasks?

@gaearon
Copy link
Collaborator

gaearon commented Jun 17, 2016

This issue is just about extracting React.DOM element factories into a separate module because they are niche and require maintaining a whitelist.

There is a separate issue about removing ReactCurrentOwner: #6350. Feel free to discuss it there!

@chicoxyzzy
Copy link
Contributor

chicoxyzzy commented Jun 17, 2016

This issue is just about extracting React.DOM element factories

Yes I understand that. Actually this is a task I'd be glad to help with. But it's not clear if there is a consensus on how to solve a problem. That's why I propose to split it into separate subtask.

@gaearon
Copy link
Collaborator

gaearon commented Jun 19, 2016

That's why I propose to split it into separate subtask.

To split what into a separate subtask? As far as I can see this is the only issue concerning this thread.

@moodysalem
Copy link

I made a module with all the factories in 15.1.0 React.DOM
https://github.com/moodysalem/react.dom.git

@chicoxyzzy
Copy link
Contributor

chicoxyzzy commented Jun 19, 2016

@moodysalem react.dom package should be in React repo (somewhere here I suppose). Also there are some validators I think we should preserve. Look here for context

@sophiebits
Copy link
Collaborator

Will fiber reconciler help to solve owner concept issue anyhow?

No, it wouldn't make a difference.

@chicoxyzzy
Copy link
Contributor

@gaearon I thought that these problems should be solved first so original issue could be splitted into subtasks.

@gaearon
Copy link
Collaborator

gaearon commented Jun 20, 2016

@chicoxyzzy Points 2 and 3 are observations, there’s nothing actionable there as far as I can see. Point 1 is a problem described in #6350.

@flarnie
Copy link
Contributor

flarnie commented May 24, 2017

I think we will be able to close this when we release 15.6. 🎈

@gaearon
Copy link
Collaborator

gaearon commented Jul 26, 2017

We did.

@gaearon gaearon closed this as completed Jul 26, 2017
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

No branches or pull requests