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

Remove all warnings in <StrictMode> #6385

Closed
jaredpalmer opened this issue Oct 9, 2018 · 8 comments
Closed

Remove all warnings in <StrictMode> #6385

jaredpalmer opened this issue Oct 9, 2018 · 8 comments

Comments

@jaredpalmer
Copy link
Contributor

RR4 beta still fails <StrictMode> because it uses the legacy context API. We should consider moving to create-react-context to prepare for future React releases.

@mjackson
Copy link
Member

mjackson commented Oct 9, 2018

We actually are using create-react-context, but we are also using the legacy context API for the sake of people who might have (against our recommendation) been using this.context.router directly.

If it's going to be a show-stopper, we can just remove the old context code. But I was hoping to ship 4.4 with both and then just print a bunch of warnings if you access the old stuff.

Another option might be to only run that code if we're running an older build of React...

if (!React.createContext) {
  Route.childContextTypes = ...
}

@jaredpalmer
Copy link
Contributor Author

AFAICT it is a showstopper off of React master at the moment

@mjackson
Copy link
Member

<StrictMode> will print warnings, but everything should run just fine on current React master. We're not actually using the old context API internally. Just keeping a copy of it there for people who might have been using it.

@mjackson mjackson changed the title [beta] <StrictMode> Remove all warnings in <StrictMode> Oct 10, 2018
@mjackson
Copy link
Member

Going to cherry-pick the <StrictMode> tests from #6287 to make sure we get rid of this problem once and for all.

@kafein
Copy link
Contributor

kafein commented Oct 12, 2018

@mjackson
What about legacy context api? If possible please remove old context stuff. If you have a time please look issue #6348, I think its related to legacy context.

@mjackson mjackson reopened this Oct 12, 2018
@mjackson
Copy link
Member

I closed this prematurely. Will keep looking into it.

@mjackson
Copy link
Member

So, I'm trying to trigger the <StrictMode> warning about legacy context usage so I can make a test that we don't trigger that warning, but I can't get the warning to fire no matter what I do.

Here's the test for the warning message I'm trying to trigger in React core: https://github.com/facebook/react/blob/4773fdf7cdf5d6d775ad0960f23ee1a830e7b82b/packages/react/src/__tests__/ReactStrictMode-test.internal.js#L819-L911

And here's the code I'm using to try to cause that warning to fire: https://codesandbox.io/s/k265vz8z8v

I've checked to make sure that <StrictMode> fires on other violations, like using componentWillReceiveProps, and that works fine. But I can't trigger the error with legacy context for some reason...

mjackson added a commit that referenced this issue Oct 13, 2018
- Prevented warnings about legacy context usage from showing up in
<StrictMode>.
- Added a few different builds of React to the repository for running
the tests under different versions of React.

Helps with #6385 and #6388
@mjackson
Copy link
Member

Hey @jaredpalmer, I decided to just go ahead and run all of our tests in <StrictMode> to make sure we weren't triggering warnings anywhere, and we're good now on master. I'm going to cut another beta today. Please let me know how it goes!

@lock lock bot locked as resolved and limited conversation to collaborators Dec 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants