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

react-is breaking change, re AsyncMode #13958

Closed
ljharb opened this issue Oct 24, 2018 · 10 comments
Closed

react-is breaking change, re AsyncMode #13958

ljharb opened this issue Oct 24, 2018 · 10 comments

Comments

@ljharb
Copy link
Contributor

ljharb commented Oct 24, 2018

It seems like react-is v16.6 no longer is compatible with React v16.3 - v16.5, in that React v16.3 - v16.5 will use Symbol(react.async_mode), but react-is's AsyncMode export will instead be Symbol(react.concurrent_mode).

It seems like the async mode symbol needs to remain the same in react-is 16.6+, and the isAsyncMode function would need to check both the AsyncMode and ConcurrentMode symbols.

@ljharb
Copy link
Contributor Author

ljharb commented Oct 24, 2018

(Note that this blocks full enzyme support of react v16.6)

@gaearon
Copy link
Collaborator

gaearon commented Oct 24, 2018

Can you explain more about the case in which it’s breaking?

@ljharb
Copy link
Contributor Author

ljharb commented Oct 24, 2018

sure - the enzyme react 16 adapter needs to maintain support for all versions of React 16. It provides a "getDisplayNameOfNode" function, which needs to be able to return the string 'AsyncMode' in React 16.3-16.5 (for <AsyncMode />), and the string 'ConcurrentMode' in React 16.6+ (for <ConcurrentMode />). It does this by comparing the .type to the AsyncMode or ConcurrentMode symbol exports from react-is.

In the 16.3-specific adapter, this isn't an issue, but it does have to lock the version of react-is to ~16.5 (which is the breaking change part) - it should be able to keep working with react v16.3 and react-is 16.anything.

If react-is v16.6+ had an AsyncMode symbol export that matched what React 16.3-16.5 did, then I think this wouldn't be a problem.

@gaearon
Copy link
Collaborator

gaearon commented Oct 24, 2018

If react-is v16.6+ had an AsyncMode symbol export that matched what React 16.3-16.5 did, then I think this wouldn't be a problem.

I think that sounds okay, we could do it. PR?

@ljharb
Copy link
Contributor Author

ljharb commented Oct 24, 2018

Sure, I'll try my hand at it!

@ljharb
Copy link
Contributor Author

ljharb commented Oct 24, 2018

#13959

ljharb added a commit to ljharb/react that referenced this issue Oct 25, 2018
@sebmarkbage
Copy link
Collaborator

This is our mistake because we should have prefixed isAsyncMode with unstable_ which the actual api was. Or not included it at all. We’re treating unstable_ effectively as private and will break between minors.

Are you ok with not supporting calls that are unstable_ in the future? Because we’ll break them several times between releases and don’t want to keep them all around in bundles.

@ljharb
Copy link
Contributor Author

ljharb commented Oct 25, 2018

Prefix or not, renaming things is still going to break consumers. However, I understand react decides to accept this breakage in minor versions.

Either way, the value of react-is is that it abstracts over these kind of changes. "Not including it at all" would have meant I had to hardcode this stuff inside enzyme, which defeats the whole purpose of react-is in the first place.

In this case, the bundle size difference is negligible - when this is the case, it seems worth trying to keep back compat, even on things react has decided are effectively private.

@sebmarkbage
Copy link
Collaborator

The alternative is that we put it behind a feature flag and don’t expose the featur at all. But then it’s much harder for people to try it out for cases where breakages are ok. It’s really hard for people in many environments to use alpha builds.

@ljharb
Copy link
Contributor Author

ljharb commented Oct 25, 2018

I understand the dilemma. In this case, the functionality hasn't changed, and I'm not sure why it was renamed - iow i'm not sure how it helps people try it out to have the name change now, when it's still going to have to change again when it gets promoted to being stable.

The tradeoffs seem to be somewhere between "difficult for people to try it out", "cause breakage in some amount of downstream tooling", or "incur a tiny additional bundle cost to maintain back compat". It's a tough call, and I'm not asking for a policy change - but in this case, the bundle size increase of #13959 seems pretty close to zero (i can't imagine it's more than a handful of bytes), and the benefit is that enzyme (and presumably other downstream tools) will be able to continue using react-is without having to hardcode things.

gaearon pushed a commit that referenced this issue Oct 31, 2018
jetoneza pushed a commit to jetoneza/react that referenced this issue Jan 23, 2019
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

3 participants