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

Supporting all of v15 without warnings #9466

Closed
jquense opened this issue Apr 20, 2017 · 6 comments
Closed

Supporting all of v15 without warnings #9466

jquense opened this issue Apr 20, 2017 · 6 comments
Assignees

Comments

@jquense
Copy link
Contributor

jquense commented Apr 20, 2017

Coming in from the Twitters, at the behest of @gaearon. This is not meant to be a discussion about what Really counts a breaking change, or how to interpret Semver. Instead I just want to share a use-case and situation that has/is causing me pain using React v15.

I have a situation with an app that is stuck at v15.1 (Before Unknown DOM prop warning). Yes it's old, yes they should upgrade. There isn't always the time, bandwidth or business case to spend time quieting the warnings. Now with v15.5 out everyone is updating libraries and apps to use prop-types instead of React.PropTypes. The problem is that prop-types doesn't work on v15 below 15.3, you get 100s of "Don't Call PropTypes" warnings. There are two issues from my perspective with this situation.

From an app author

My options are to chose between which warning I want flooding the console, (Unknown prop, or Don't call PTs). Most libraries aren't considering the change to prop-types a major bump (reasonable!), so any feature and patch updates are getting rolled in with the prop-type fix. So To avoid warnings I need to pin all my react component libraries and miss out on bug fixes, or deal with warnaggedon until there is bandwidth to upgrade, everything to react@latest

From a library maintainer

This has been covered at length before, but the pressure to fix deprecation warnings in libraries immediately is real, and stressful. In this case the upgrade path for prop-types is super easy (thanks codemod!) so you do it and release. Without realizing it though you've actually just upped the minimal required react peer of your library to 15.3, since anything below that (>0.14) will warn forever. That's a tough spot to be in, while the swap of the dep is not a breaking change, changing the minimal React version required feels more like one.

What even do we do!

I don't know! It's a hard problem. For me tho I think a reasonable rule of thumb might be that a library using only public APIs should be able to support an entire major version of React without unfixable deprecation warnings. If React major versions happened faster I'd be for a introduce warnings on major bumps, and remove them on the next major bump. We do that in react-bootstrap and it's annoying and i'm impatient but it is a strategy that has worked out well for users. The system feels fair, and no one gets new warnings introduced accidentally by a fresh npm install.

Thanks ya'll overall warnings are great, I've always appreciated how much care and time React takes to tell me what's going on, and what I need to fix.

@gaearon
Copy link
Collaborator

gaearon commented Apr 20, 2017

I have a situation with an app that is stuck at v15.1 (Before Unknown DOM prop warning). Yes it's old, yes they should upgrade. There isn't always the time, bandwidth or business case to spend time quieting the warnings.

Would 15.6 deduplicating "unknown prop" warning and only showing it for the first detected callsite solve the issue? This feels more like an omission that should have been fixed right after we added this warning. It's a bit silly if the reason people don't update past 15.1 is because of a spammy warning, and I think we should fix it.

For me tho I think a reasonable rule of thumb might be that a library using only public APIs should be able to support an entire major version of React without unfixable deprecation warnings.

I agree it's a reasonable guideline for the future. The 15.x line was very hard to manage because of all the internal implicit dependencies we have been untangling one by one. We figured it's better to sacrifice some pain now in order to avoid dragging all such issues into 16 and beyond. I don't expect to have similar issues in the future, and we'll take this guideline seriously.

If React major versions happened faster I'd be for a introduce warnings on major bumps, and remove them on the next major bump.

This is also something we've been considering, but so far our impression is people hate major releases even if they don't contain a lot (see Angular 4). In addition everyone has to update their peer deps which is annoying across the ecosystem. Maybe we'll revisit this after 16 or 17.

That's a tough spot to be in, while the swap of the dep is not a breaking change, changing the minimal React version required feels more like one.

You could bump your major in this case. i.e. React Bootstrap 42.0 emits warnings with React 15.x lineup, and React Bootstrap 43.0 completely fixes them but requires compatible React versions. Please note that we even released a 0.14.x update so even people stuck on the previous major can still upgrade as long as they are happy using the latest minor on their major (0.14.9 or 15.5+).

The only real barrier I see here is that people avoid updating minors because of too noisy warnings like DOM property one. So the fix I see here is to make sure it's only printed once in React 15.6. I think it's a reasonable compromise. WDYT?

@jquense
Copy link
Contributor Author

jquense commented Apr 20, 2017

This is also something we've been considering, but so far our impression is people hate major releases even if they don't contain a lot (see Angular 4).

I agree generally with this, but I think React may avoid this problem being at v16 already, we are pretty used to it. I do think the Angular crowd is probably coming from a different place as well, where major bumps are more sacred, but React hasn't really ever had that mystique about it's versions.

You could bump your major in this case. i.e. React Bootstrap 42.0 emits warnings with React 15.x lineup,

Yeah that's reason in some cases and probably preferable. It's unlikely from the app perspective tho that every library author will do that. (plus the same concern over folks not liking major bumps applies here) Also in this case the Peer bump is implicit and "hidden" half way down the prop-types readme.

The only real barrier I see here is that people avoid updating minors because of too noisy warnings like DOM property one. So the fix I see here is to make sure it's only printed once in React 15.6. I think it's a reasonable compromise. WDYT?

Yes definitely 👍. I'm fine as a app maintainer dealing with one warning reminding me about this, its easier to convince a dev team that the warning is fine than all users of libraries I maintain :P

@gaearon
Copy link
Collaborator

gaearon commented Apr 20, 2017

@flarnie will look into this (deduping DOM property warning) as part of getting 15.6 out.

@flarnie
Copy link
Contributor

flarnie commented Apr 20, 2017

"I have a situation with an app that is stuck at v15.1 (Before Unknown DOM prop warning). Yes it's old, yes they should upgrade. There isn't always the time, bandwidth or business case to spend time quieting the warnings."

"Would 15.6 deduplicating "unknown prop" warning and only showing it for the first detected callsite solve the issue? This feels more like an omission that should have been fixed right after we added this warning. It's a bit silly if the reason people don't update past 15.1 is because of a spammy warning, and I think we should fix it."

I am curious what the use case is that causes this warning to get logged so many times that it's a nuisance?

Passing non-standard props into a 'div' or 'span' seems like it would only come up when a parent spreads it's props into a non-custom component, like so:

return <span {...this.props} style={styles.handle}>{this.props.children}</span>;

Am I missing a more common situation where it happens?

I imagine that some application authors might prefer to do this everywhere, because then when a distant leaf receives data from the root of the tree, and the data format changes you don't have to make changes to all the components in between. This is part of the problem that Redux, Relay, etc. address. Even in that case, the places rendering non-custom components like 'div' and 'span' could still pick out the specific props they need.

Still going to work on a PR for this, just hoping to understand more the root cause of the issue.

I think the concern for library authors makes sense - that is an issue with any adding of warnings in minor versions of React, which then may show up in libraries that use React.

@gaearon
Copy link
Collaborator

gaearon commented Apr 21, 2017

Third party component libraries used to often "spread everything" because it's less typing. This caused literally hundreds of warnings for people using e.g. React Bootstrap. While we want people to fix those it's really overwhelming and scary especially if you have no control over that library, and causes panic in issue trackers of those libraries.

flarnie added a commit to flarnie/react that referenced this issue Apr 22, 2017
Some widely used libraries which depend on React have used syntax that
ends up passing non-standard props to default React components like
'dom' and 'div'. For example:
```
  // where this.props contains attributes like 'foo'
  return <div {...this.props} />;
```

Since we started throwing warnings for this syntax in Reactv15.2 some
libraries have not upgraded past v15.2.

Now, because of deprecations in 15.5, some libraries will only work with
Reactv15.5, and this causes a conflict when other libraries are pinned
at v15.1. The overall effect is that either there are version conflicts
or the "unknown prop" warning is thrown all over the place, and users of
these libraries can't fix the warnings.

See facebook#9466 for more context.

We are deduping this warning in hopes that it allows more library
authors to update to the latest version of React. React v16.0 may take a
different approach to this issue. For some related discussion, see
facebook#9477

**Test Plan:**
 - Added a unit test
 - Manually tested with a fixture that was not committed; https://gist.github.com/flarnie/db011bf54206f30b9983cd4dc674c82e
@gaearon
Copy link
Collaborator

gaearon commented May 8, 2017

We have debated this internally for a while, and have not really reached a consensus, so I’m going to close it for now. I’m sorry we are not addressing this sooner, but instead of being inconsistent we would prefer to address the issues with the warning system separately in the future.

@gaearon gaearon closed this as completed May 8, 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

3 participants