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

[FIX] - ReactDOM Add back version property to client #15780

Merged

Conversation

ealush
Copy link
Member

@ealush ealush commented May 30, 2019

Change details

Adding back .version property to ReactDOM. It used to exist in React15 and got deleted. It still exists in ReactDOM/server, and it appears to be typed in @typed/react-dom so I am assuming it got deleted by mistake.

@sizebot
Copy link

sizebot commented May 30, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@ealush ealush changed the title Add version property to ReactDOM [FIX] - ReactDOM Add back version property to client Jun 11, 2019
@ealush
Copy link
Member Author

ealush commented Jul 23, 2019

Ping:
@threepointone, @trueadm
Can you take a look at it? It can really help when debugging version mismatch.

@michasherman
Copy link

Any updates on this?

@omrilotan
Copy link

Is this going to be added?

@ealush ealush force-pushed the 2019-05-30-add-version-to-react-dom branch from 0d010e4 to a4f9d91 Compare January 23, 2020 22:39
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a4f9d91:

Sandbox Source
nifty-framework-qigqk Configuration

@ealush ealush requested a review from gaearon January 23, 2020 22:43
@sizebot
Copy link

sizebot commented Jan 23, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against a4f9d91

@sizebot
Copy link

sizebot commented Jan 23, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against a4f9d91

@milesj
Copy link
Contributor

milesj commented Jan 23, 2020

Why is this necessary? Just do require('react-dom/package.json').version.

@ealush
Copy link
Member Author

ealush commented Jan 24, 2020

@milesj, First, this is the browser interface, in the browser I don't have direct access to package.json. You can expose it, of course, but when debugging production applications this is not something you want to do.

Second, as I noted in the description of this pr All other interfaces (react, react-dom/server) still have it, and this is the only one where it doesn't exist. @typed also assumes it is there.

All these lead me to believe that the version key got deleted by mistake when moving from react 15 to 16.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the decision to remove it was intentional or not.

React DOM reports its version to DevTools (which necessarily needs some forking, since it supports many versions) but I think we might be cautious about having application code check this number. Usually it's better to feature detect specific APIs instead.

I'm a little reluctant to add it back because I'm not sure how you're wanting to use it 😄 Tell me more?

cc @gaearon and @sebmarkbage

@ealush
Copy link
Member Author

ealush commented Feb 3, 2020

TBH I had no idea you could detect ReactDOM's version from devtools.

When I created the PR we were debugging a production site with multiple versions of React present on the same page - some with legacy React15 code and some with React16. We experienced a React/ReactDOM version mismatch which resulted in hooks errors.

When trying to debug it on the server both react and react-dom exposed their version, but in the client only react did so we couldn't really tell which version of React our code was using. The errors were not present in development.

Working on this issue I assumed it got deleted by mistake since all other APIs still maintained the version property. If that's something that we don't plan on adding back then maybe I should create an opposite PR to fix DefinitelyTyped 😅.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems legit to bring the client into parity with the others. I ran this change by the team and there were no objections.

Note that the version reported for experimental releases will be a little different, e.g. NPM version 0.0.0-experimental-241c4467e will be reported as "16.12.0-experimental-241c4467e".

@bvaughn bvaughn merged commit 9944bf2 into facebook:master Feb 3, 2020
@ealush ealush deleted the 2019-05-30-add-version-to-react-dom branch February 3, 2020 20:39
@gaearon
Copy link
Collaborator

gaearon commented Feb 26, 2020

Out in 16.13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants