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

feat(Transition): Add findDOMNode prop #457

Closed
wants to merge 3 commits into from

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Feb 14, 2019

Useful for strict mode compatibility. Part of #429.

Started out with something along the lines of a skipDOMNode but this would've been equivalent to <Transition findDOMNode={() => undefined} />. Current solution allows more flexibility by preserving the call signature of the state transition callbacks while being strict mode compatible with regards to findDOMNode deprecation.

Not sure how/if I should update the docs.

@taion
Copy link
Member

taion commented Feb 14, 2019

Is this the path forward that we want? I'd imagine that reactjs/rfcs#97 would supersede this in a nicer way.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Feb 14, 2019

Is this the path forward that we want? I'd imagine that reactjs/rfcs#97 would supersede this in a nicer way.

At which point we drop ReactDOM.findDOMNode and use that rfc. This approach doesn't contradict this. It is a bridge until a better solution is found. That RFC however isn't even accepted yet or any implementation planned so I don't get my hopes up.

@taion
Copy link
Member

taion commented Feb 14, 2019

It'd be a breaking change to remove this API, though.

@jquense
Copy link
Collaborator

jquense commented Feb 14, 2019

I think in the short term we probably just want to switch to full ref props, and cut a breaking release. I'm not in a huge rush for that tho, but we can make breaking releases. The reticence here to add new public api's is that this is all to work around a warning in strict mode, not an actual bug. It's just generally lower priority to fix, especially when weighed against public API changes required

@eps1lon
Copy link
Collaborator Author

eps1lon commented Feb 14, 2019

It'd be a breaking change to remove this API, though.

Sure. But I wasn't the one to suggest a (maybe) future feature of react.

The reticence here to add new public api's is that this is all to work around a warning in strict mode, not an actual bug.

I see this argument all the time: "It's just a warning". This is just not realistic for library authors. Especially if this is a transitive dependency. There is currently no way in react development to filter out deprecation warnings, prop types warnings or internal react warnings. These are all the same. A runtime warning in transitive dependencies is considered a bug by many people. It adds at least a lot of noise that is indistinguishable from actual errors in the console.

[...] in strict mode, not an actual bug.

Strict mode and concurrent mode. I'm not sure how findDOMNode behaves in that mode. Might be a bug there. It's especially a bug while testing. mocha will not let tests pass for any warning that is issued by react because they use console.error.

API changes required

It's an addition with virtually zero runtime overhead and (fairly certain) no bundle size increase. If you're not interested in being strict or concurrent mode ready then I can close this.

@jquense
Copy link
Collaborator

jquense commented Feb 14, 2019

These are all the same. A runtime warning in transitive dependencies is considered a bug by man people. It adds at least a lot of noise that is indistinguishable from actual errors in the console.

Believe me, I HEAR you on this, I maintain a ton of react libraries. The plain fact tho is this is not a bug and it doesn't benefit us as maintainers to accept the incorrect claim that they are from some users. React, itself holds this position, is explicitly about how these are not bugs. It's not a tenable position for maintainers to have to drop everything on every minor and patch version of react to silence new warnings,

There is currently no way in react development to filter out deprecation warnings, prop types warnings or internal react warnings.

These are strict mode warnings so are entirely opt in.

If you're not interested in being strict or concurrent mode ready then I can close this.

We very interested in being ready! I even offered an approach above. This is not about refusing to the fix the problem, but in fixing it in a sustainable way and with a look to future proofing, API churn reduction etc. We want to fix this, but there is currently no pressing rush to do so that we need to pick a less optimal solution.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Feb 14, 2019

Sounds fair to me. Let's say we might be able to determine in a component if we're in a strict mode tree. Would you be open to skip the findDOMNode call if a given Transition component is a child of React.StrictMode?

Edit: Probably not good either. Causes the component to run different in development and production.

@jquense
Copy link
Collaborator

jquense commented May 5, 2020

🎉 This issue has been resolved in version 4.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@eps1lon eps1lon deleted the feat/skip-findDOMNode branch May 5, 2020 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants