React 15.4 compatibility; don't rely on internal modules

[STRML]

Setting style is easy enough to do with standard DOM apis,
and the shouldComponentUpdate is unnecessary as render is
extremely quick.

[daltones]

@STRML actually it's not that easy. CSSPropertyOperations.setValueForStyles() does a few more things than setting DOM style.

For example, I think your solution won't work with plain number values that React coerces to strings with px unit.

So this PR would be a breaking change.

[STRML]

That's true. We could write that in or rely on a module. There are likely other edge cases as well; how far do we want to go to cover them all?

[daltones]

In my opinion, a better solution would be a new major release without support to setting className and style to the portal container. It would be very easy for developers to put another div wrapper for the content (and set every other prop that has no support now).

That container is created outside React, so I guess it's more rational to not rely on it at all.

[STRML]

That sounds good to me. I've updated the PR to remove className/style support entirely.

[STRML]

Whoops, forgot to update the tests. CI should be green now.

[daltones]

Thanks @STRML!

Just a note about the drop of shallowCompare: I think that in almost all cases, that was useless.

Probably we would use react-portal like this:

// [...]
return (
  <Portal closeOnEsc closeOnOutsideClick>
    <div>
      <h2>Title depending on {this.props.someProp}.</h2>
      <p>Another thing depending on {this.state.someState}.</p>
    </div>
  </Portal>
);

So the prop children of the Portal component would change every time! That's true for all components that works with children.

Except for edge cases like this:

const NEVER_CHANGE_CHILDREN = (
  <div>
    <p>Really static content.</p>
  </div>
);

// [...]

return (
  <Portal closeOnEsc closeOnOutsideClick>
    {NEVER_CHANGE_CHILDREN}
  </Portal>
);

So this drop is really like nothing in terms of rendering.

[STRML]

It would be less useless in combination with babel-plugin-transform-react-constant-elements; however, since the render() is basically a noop in most cases, I'm not sure it even helps even in that best case.

[tajo]

Thanks! I agree with removing applyClassNameAndStyle. I've never relied on that functionality anyway and there are other ways how to set the styles. I'm going to release it as v3.

[daltones]

Thanks @tajo! 😃

[slorber]

Hey.

I don't really undestand why style/className support has been removed. Personnally I rely on this, and I find it more convenient to have a className directly on the portal node than on a child node. How am I supposed to position the portal correctly if I can't craft a selector on it?

Are you all using position fixed on a portal's child? Because I absolutly can't do that on my own app, so I need correct positionning of the portal.

Removing usage of internal modules does not seem related to me to supporting className and style attributes. Why breaking compatibility on this?

Also, why remove this line ? this.node.className = props.className; it does not rely on any internal React code so it was not a big deal to leave it in the lib, and would keep being retrocompatible for most users like me (not so many are using pure inline styles currently)

[slorber]

I've made a PR to support className again
#113

[slorber]

Also I'd like to mention that stuff like shouldComponentUpdate shouldn't have to be removed too IMHO. (but I'm not sure it's that useful anyway because for portals it's likely the children will always change, and it's the children that should rather optimize rendering directly)

If you want to remove React internal code usage it's fine, but it does not mean to introduce unnecessary breaking changes. I'm sure most lib users would be fine if you copy some lines of the react internal code you rely on to maintain compatibility (as a temporary solution, not saying lib size is not important). I'd rather have a little bigger lib than a non-compatible lib :)

But in this case I'm not sure it's really worth it. shouldComponentUpdate is not that useful for portals, and I don't think many people use JS inline styles with portals.

(However I think it would be nice to support CSS-IN-JS frameworks like styled/glamor and allow to pass className + data-attributes to portal node.)

[daltones]

@slorber I guess this decision became from “just support React 15.4” to something like “improve the library design”. And that's the main reason for the new major release with breaking changes.

And, of course, all correction needed is pretty obvious:

<Portal className="my-class" style={{ color: '#F00' }}>
  {content}
</Portal>
// becomes:
<Portal>
  <div className="my-class" style={{ color: '#F00' }}>
    {content}
  </div>
</Portal>

The portal wrapper is created directly with some document.createElement(), it's not managed by React. So the main principle here is to keep it untouched. It's like the ReactDOM.render() method — you will always have your own root container.

It's not good that the wrapper looked like a normal element (that you can set className and style) and them you try to set some onClick and it won't work. Now you have to have your own container, but it will have full support of React.

[slorber]

@daltones I do not agree with that decision and I think it should rather be discussed with the community.

Personnally I'm interested to have both React 15.4.1 support, and className support for my app, but with 3.0.0 release it was simply not possible.

Also I think it's really annoying to have "divs" with no attributes while debugging your dom because you can't see immediately it's a portal and which one it is. When your app may have a lot of portals at the same time it's quite annoying.

Also it's not consistent with what other portal libs are doing, as these solutions allow classnames (and sometime styles) on the portal node:

• https://github.com/cloudflare/react-gateway
• https://github.com/souporserious/react-tether
• https://github.com/reactjs/react-modal

I would also say that if people have different opinions on the subject, it's worth covering everybody's opinion. If you think it's not elegant to put a className on a portal node, then don't, but please let me do it :)

[daltones]

Okay, let's see what @tajo says about rolling back className support.

But I don't think we're trying to reach some consistency with those libraries at all. I mean, consistency with what? They will not work together, they're just different solutions. We tried to follow the principle of separate concerns here: the portal does its job rendering something somewhere else; that thing styles itself. That's it.

[imjohansunden]

I agree with @slorber . If you don't want the portal to have a class, do not add it. But let me decide if I want to add a class to mine. The example above would in my case look like:

<Portal className="my-class" onOpen={this.onOpen} beforeClose={this.beforeClose} isOpened={true} closeOnEsc closeOnOutsideClick>
    // content
</Portal>

I understand what you are getting at with the principle of concern @daltones. But in this case I do not want to have an "unnamed" div in the DOM because when we are building our framework we need to give classes to all of the framework elements.

[imjohansunden]

This also forces me to add a ref to the extra div for animation in for example beforeClose. It was easier before because you recieved the node passed in as an argument and could use it directly for animation. Just as shown in the examples. I still recieve the node but now I need to get the DOM node for the new div.

[STRML]

I'm sorry this disrupted your workflow, but breaking changes is the entire point of a major release.

We found that by disabling className/style support we could significantly shrink the size of this library by more than 50% including dependencies - there is no guarantee that react/lib functions will continue to exist, and using them is generally fundamentally incompatible with Preact/Inferno.

Re: shouldComponentUpdate, I am not sure what your point is. You say that it shouldn't be removed, then admit it wasn't that useful. We should not be in the business of shipping unnecessary code.

Portal certainly will not and should not natively support css-in-js frameworks. It's a matter of scope: do one thing, do it well. The whole point of composable components is centered around this concept.

It's not good that the wrapper looked like a normal element (that you can set className and style) and them you try to set some onClick and it won't work. Now you have to have your own container, but it will have full support of React.

Exactly. The root component was fundamentally broken as a pseudo-DOM element; it only supported className and style, and setting and resetting them required reusing internal React code. It simply wasn't needed.

I'm sorry this caused you all some work, but the result is that Portal is a much more predictable and svelte library now.

[tajo]

I am with @STRML on this. I think that adding className was a mistake and I should have not let it in. It caused some bugs and confusion. It doesn't seem that many people mind a wrapper div. I'm trying to keep this lib as lean as possible and for single-purpose only.

[slorber]

Actually I'm not really discussing about anything else than className. I'm ok for removing shouldComponentUpdate and having a breaking change on inline styles if that's complicated, but I'd like to be able still use a className:

• No breaking change for majority of users
• No need for extra div
• Much easier to debug portal problems in devtools for me without having to unfold divs
• Allow to detect easily from anywhere if any portal is open with a simple selector like body > div.portalClassName
• Passing className will also make CSS-in-JS solutions work out of the box
• Does not need more than 10 lines of code to support it, without any external dependency

Also sometimes @tajo we work in teams where the integration team has done some CSS, and if I come to them and tell them "I'm sorry the JS lib I'm using forces me to create an extra useless wrapper div element with no attributes, you have to rewrite some selectors because of the lib I decided to use", it's not cool (and yes, we may use body > div.portalName selector)

Personnally I don't think it's a good idea to consider that the node on which you mount the portal is to be considered as equivalent to the react mount node. There is only one react app, and it is known to have a single mount node. The portals are implementation details of that react app, so why should they be considered somehow as different extra react apps inside my app from an exterior point of view?

As @STRML seems to be ok to support className what about his suggestion of warning when passing style/onX props that are unexpected, so that the user knows and isn't surprised about unsupported properties?

Would this get merged?

[tajo]

No breaking change for majority of users

If it was used a lot there would be more people here talking about that (the lib has now 50k/month downloads...).

No need for extra div

Not really a big deal. React devs create a lot of extra divs anyway.

Much easier to debug portal problems in devtools for me without having to unfold divs

👌

Allow to detect easily from anywhere if any portal is open with a simple selector like body > div.portalClassName

Could be a className attached to your extra/root div? Besides, this doesn't seem as a valid use-case anyway (maybe if you combine React with some other libs).

Passing className will also make CSS-in-JS solutions work out of the box

Can you explain? What's the use case? What's the "not out of the box" solution?

Does not need more than 10 lines of code to support it, without any external dependency

That's not an argument. At all.

Would this get merged?

No, sorry. Unless there are 10+ people up-voting that and having better arguments. Then, I would think about that again.

[slorber]

@tajo I understand your points but just these arguments should be largely enough:

• No useless extra div element
• No breaking change
• Easier to debug
• Adding back the feature is very lightweight and does not break anything

I agree that this feature is not absolutely required to build portals, and my request is mostly for developer experience and ease of migration, but by doing this change, you make the React 15.4 migration path a bit harder for those of us who use a lot of complex portals in our app. In my situation, I'm unable to upgrade React without refactoring all my portals CSS, taking the risk of breaking things. And yes, on a complex legacy app, it can be significantly more difficult than just adding a new extra div and changing 1 css selector.

The release is very recent, so let's see what people think over time, in the meantime, here's a fork people can use if they want to migrate to React 15.4.0 without having to refactor their portals:

"react-portal": "https://github.com/slorber/react-portal.git#build",

[slorber]

Also note that the Portal component only passes down the "closePortal" callback if the element is not a dom element, which means that

<Portal className="clazz">
  <PortalContent/>
</Portal>

Can't be replaced out of the box by

<Portal>
  <div className="clazz">
    <PortalContent/>
  </div>
</Portal>

Otherwise the PortalContent would never receive that callback.

A general solution can be:

<Portal>
  <PortalWrapper className="clazz">
    <PortalContent/>
  </PortalWrapper>
</Portal>

Where PortalWrapper is:

const PortalWrapper = ({
  children,
  closePortal,
  className,
  ...rest
}) => {
  let childrenUpdated = children;
  if (typeof childrenUpdated.type === 'function' && closePortal) {
    childrenUpdated = React.cloneElement(childrenUpdated, {closePortal: closePortal});
  }
  return (
    <div
      className={classnames('portal', className)}
      {...rest}
    >
      {childrenUpdated}
    </div>
  );
};

For 10 lines of code removed in the lib, you make the React upgrade more painful and risky.

I'm not saying you should not remove the className support (I agree that unability to add a onClick listener is disturbing and I've tried to do it), but at least please make a 3.x release with it, and maybe remove it later in 4.x. People could at least still upgrade React without too much pain, and decide for themselves if they want to upgrade lib to 4.x if they don't absolutely need className support.

[devinmcinnis]

Since this has been referenced from multiple issues, I want to add that even though react-portal does not allow setting a class name, it would be beneficial to have a class name for the component.

Currently, there is no way to target the containing <div> wrapper.

My use case is supporting a modal on iOS Safari. (CSS) Viewport units aren't functional/reliable with the expanding/collapsing menus and without being able to add height: 100% to the react-portal <div>, it's impossible to properly support.

[STRML]

@devinmcinnis See https://github.com/STRML/react-portal-minimal, where I've rewritten this component. It supports className.

[slorber]

@STRML I'm not sure to understand the situation :D

You were the one removing className support in this lib and said the following:

The root component was fundamentally broken as a pseudo-DOM element;

Yet you support className in your forked library? why ?

[STRML]

I changed my view - I basically wanted to get the root element out of any business of passing along normal DOM attributes, but I found that className was simply too useful not to have because of various browser quirks/limitations.

See also #113 (comment). The point is to stop the support of DOM attributes at className, so at least it can easily be targeted by CSS without delving too deep into browser quirks or having to rely on DOM manipulation libs.

[slorber]

Hmm yeah didnd't remember your comment here ;)