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

Deprecate context object as a consumer and add a warning message #13829

Merged
merged 14 commits into from Oct 12, 2018

Conversation

Projects
None yet
6 participants
@trueadm
Contributor

trueadm commented Oct 11, 2018

This PR deprecates usage of React context as a consumer in DEV mode, along with a warning message informing the user of this change. Under the hood, we create a new object for context.consumer rather than having a cyclic reference to context. This object proxies values from the original context so it remains backwards compatible in React 16.

dev when using consumer:

  • Before: When using Context.Consumer, it's the same as Context so nothing special is handled.

  • After: When Context.Consumer is used the at the beginning of reconciliation, the fiber is given the _context field from the Consumer, that references to Context.

dev when using context:

  • Before: When using Context, it's the same as the previous case (but the wrong behaviour).

  • After: When Context we check if _context field exists on it (which we added to the new proxy object) and because it won't (only Context.Consumer does) we trigger a new warning.

prod when using consumer:

  • Before & After: When using Context.Consumer, it's the same as Context so nothing special is handled.

prod when using context:

  • Before & After: When using Context, it's the same as the previous case so nothing special is handled.
context.Consumer = {
$$typeof: REACT_CONTEXT_TYPE,
_context: context,
get _calculateChangedBits() {

This comment has been minimized.

@gaearon

gaearon Oct 11, 2018

Member

I think getters don't work on www, can you rewrite this as Object.defineProperty calls?
Worth checking tho

This comment has been minimized.

@trueadm

trueadm Oct 11, 2018

Contributor

Really? They work in IE9 the last time I checked.

This comment has been minimized.

@trueadm

trueadm Oct 11, 2018

Contributor

I changed over to Object.defineProperties. It's a shame Flow has such bad support for them though :(

@sizebot

This comment has been minimized.

sizebot commented Oct 11, 2018

React: size: -0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: 0af8199...9149b17

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +2.7% +1.9% 92.51 KB 94.99 KB 24.42 KB 24.89 KB UMD_DEV
react.production.min.js -0.0% 0.0% 11.87 KB 11.87 KB 4.67 KB 4.67 KB UMD_PROD
react.development.js +4.5% +3.3% 55.09 KB 57.57 KB 14.92 KB 15.42 KB NODE_DEV
react.production.min.js -0.0% 0.0% 6.28 KB 6.28 KB 2.66 KB 2.66 KB NODE_PROD
React-dev.js +5.1% +3.9% 51.45 KB 54.08 KB 14.08 KB 14.63 KB FB_WWW_DEV
React-prod.js -0.1% -0.0% 14.37 KB 14.35 KB 3.93 KB 3.93 KB FB_WWW_PROD
React-profiling.js -0.1% -0.0% 14.37 KB 14.35 KB 3.93 KB 3.93 KB FB_WWW_PROFILING
react.profiling.min.js -0.0% 0.0% 14.02 KB 14.02 KB 5.19 KB 5.19 KB UMD_PROFILING

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.2% 656.95 KB 657.9 KB 153.51 KB 153.84 KB UMD_DEV
react-dom.development.js +0.1% +0.2% 652.29 KB 653.24 KB 152.13 KB 152.46 KB NODE_DEV
ReactDOM-dev.js +0.1% +0.2% 669.21 KB 670.2 KB 152.73 KB 153.05 KB FB_WWW_DEV

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.3% 446.75 KB 447.7 KB 99.89 KB 100.21 KB UMD_DEV
react-art.development.js +0.2% +0.4% 378.54 KB 379.48 KB 82.71 KB 83.04 KB NODE_DEV
ReactART-dev.js +0.3% +0.4% 382.2 KB 383.19 KB 81.12 KB 81.45 KB FB_WWW_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.2% +0.4% 390.79 KB 391.73 KB 85.36 KB 85.68 KB UMD_DEV
react-test-renderer.development.js +0.2% +0.4% 386.36 KB 387.31 KB 84.24 KB 84.56 KB NODE_DEV
ReactTestRenderer-dev.js +0.3% +0.4% 390.42 KB 391.41 KB 82.97 KB 83.3 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.3% +0.4% 374.35 KB 375.3 KB 80.83 KB 81.15 KB NODE_DEV
react-reconciler-persistent.development.js +0.3% +0.4% 372.96 KB 373.9 KB 80.26 KB 80.59 KB NODE_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.3% 506.42 KB 507.41 KB 111.76 KB 112.09 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.2% +0.3% 506.13 KB 507.12 KB 111.68 KB 112.01 KB RN_OSS_DEV
ReactFabric-dev.js +0.2% +0.3% 496.59 KB 497.57 KB 109.33 KB 109.66 KB RN_FB_DEV
ReactFabric-dev.js +0.2% +0.3% 496.62 KB 497.61 KB 109.35 KB 109.68 KB RN_OSS_DEV

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

}
} else {
// Context.Consumer is a separate object in DEV, where
// _context points back to the original context

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

This comment confuses me. It talks about DEV but is in PROD code path?

This comment has been minimized.

@trueadm

trueadm Oct 12, 2018

Contributor

It's a DEV path.

false,
'You are using the Context from React.createContext() as a consumer.' +
'The correct way is to use Context.Consumer as the consumer instead. ' +
'This usage is deprecated and will be removed in the next major version.',

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

"in a future major release"

} else {
// Context.Consumer is a separate object in DEV, where
// _context points back to the original context
context = workInProgress.type._context;

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

Can you explain why we read it differently in DEV and PROD? It would help to have a written explanation of DEV/PROD matrix for Consumer/Context object and how behavior changes for each.

This comment has been minimized.

@trueadm

trueadm Oct 12, 2018

Contributor

Can you reference me to another matrix? I updated the comment

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

There's none. I just meant if you could write up of what's supposed to change.

  • dev when using consumer: (before) and (after)
  • dev when using context: (before) and (after)
  • prod when using consumer: (before) and (after)
  • prod when using context: (before) and (after)

That would be easier for me to review since I can focus on intent separately from code.

This comment has been minimized.

@trueadm

trueadm Oct 12, 2018

Contributor

I've put a big nice comment in there that explains things better now. I'll aadd the matrix to this PR.

Provider: context.Provider,
};
// $FlowFixMe: Flow complains about not setting a value, which is intentional here
Object.defineProperties(consumer, {

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

Is it better to have consumer proxy to context, or the other way around? Why?

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 12, 2018

Member

Whatever we read/write from/to most. I’m actually not sure which one that is.

This comment has been minimized.

@trueadm

trueadm Oct 12, 2018

Contributor

I checked and the context gets written to the most in our tests.

let context: ReactContext<any> = workInProgress.type;
// The logic below for Context differs depending on PROD or DEV mode. In
// DEV mode, we create a separate object for Context.Consumer that acts
// like a proxy to Context. This proxy object adds unecessary code in PROD

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

unnecessary

// a property called "_context", which also gives us the ability to check
// in DEV mode if this property exists or not and warn if it does not.
if (__DEV__) {
if (workInProgress.type._context === undefined) {

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

Can this just be context._context? To avoid extra reads (even in DEV).

);
}
} else {
context = workInProgress.type._context;

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

This can also be just context._context.

ReactNoop.flush();
}).toLowPriorityWarnDev(
'You are using the Context from React.createContext() as a consumer.' +
'The correct way is to use Context.Consumer as the consumer instead. ' +

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

This message might confuse people a little bit. Maybe make it more visual?

Rendering <Context> directly is not supported and will be removed in a future major release. Did you mean to render <Context.Consumer> instead?
'You are using the Context from React.createContext() as a consumer.' +
'The correct way is to use Context.Consumer as the consumer instead. ' +
'This usage is deprecated and will be removed in a future major release.',
{withoutStack: true},

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

Why no stack? This is exactly the kind of warning where I think the stack would be highly valuable.

This comment has been minimized.

@trueadm

trueadm Oct 12, 2018

Contributor

n00b question: how does one generate a stack with lowPriorityWarning? I know you can with warning but then the APIs are consistent then. I'll just switch to warning to unblock this.

},
Consumer: {
get() {
return context.Consumer;

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

What if you render <Context.Consumer.Consumer>? Seems like that should also warn because it also relies on objects being shared.

Which makes me think: should the warning move into these getters instead? Fire for first getter accessed, ignore the rest. This could also nicely let us warn once per context type.

This comment has been minimized.

@trueadm

trueadm Oct 12, 2018

Contributor

Moving the warning into the getters will stop the error from coming up for just using <Context /> as React's internals will always read/write from the context and never the consumer – the backwards compatibility is there for cases where libraries might try and read/write to the consumer for whatever reason. I'll address the nested Consumer.Consumer issue.

};
// $FlowFixMe: Flow complains about not setting a value, which is intentional here
Object.defineProperties(consumer, {
_calculateChangedBits: {

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

_calculateChangedBits and unstable_read never change, can we directly point to the original without getters for them?

Provider: context.Provider,
unstable_read: context.unstable_read,

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

It's assigned right after, no? This is undefined.

This comment has been minimized.

@trueadm

trueadm Oct 12, 2018

Contributor

Moved the line above, my bad.

_context: context,
_calculateChangedBits: context._calculateChangedBits,
Provider: context.Provider,
unstable_read: context.unstable_read,

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

Now that I think of it Context.Consumer.unstable_read() should also warn.

@trueadm

This comment has been minimized.

Contributor

trueadm commented Oct 12, 2018

I'll do that change in a follow up on Monday. :)

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 12, 2018

Cool. Don't forget a client-side test though. It's easy to change a field name, forget to update the proxy, and break it.

@gaearon gaearon deleted the trueadm:deprecate-context-object-as-consumer branch Oct 12, 2018

linjiajian999 pushed a commit to linjiajian999/react that referenced this pull request Oct 22, 2018

Deprecate context object as a consumer and add a warning message (fac…
…ebook#13829)

* Deprecate context object as a consumer and add various warning messages for unsupported usage.

@gaearon gaearon referenced this pull request Oct 23, 2018

Merged

Add 16.6.0 changelog #13927

@jaydenseric

This comment has been minimized.

jaydenseric commented Oct 25, 2018

React v16.6 had some breaking changes to the way context works for SSR libraries. I'm trying to fix jaydenseric/graphql-react#11 but finding it pretty hard to work out whats going on… any tips?

Here are some relevant locations:

jaydenseric added a commit to jaydenseric/graphql-react that referenced this pull request Oct 25, 2018

Update react peer dependency to >= v16.6.
The fixes in response to facebook/react#13829 break support for react < v16.6.
@jaydenseric

This comment has been minimized.

jaydenseric commented Oct 25, 2018

I thought I'd worked it out, but discovered (after a release and deployment 😞) that context consumer behavior is pretty different when NODE_ENV=production:

{ '$$typeof': Symbol(react.element),
  type:
   { '$$typeof': Symbol(react.context),
     _context:
      { '$$typeof': Symbol(react.context),
        _calculateChangedBits: null,
        _currentValue: undefined,
        _currentValue2: undefined,
        Provider: [Object],
        Consumer: [Circular],
        _currentRenderer: null,
        _currentRenderer2: null,
        displayName: 'GraphQLContext',
        currentValue: [GraphQL] },
     _calculateChangedBits: null },
  key: null,
  ref: null,
  props: { children: [Function] },
  _owner: null,
  _store: {} }

vs

{ '$$typeof': Symbol(react.element),
  type:
   { '$$typeof': Symbol(react.context),
     _calculateChangedBits: null,
     _currentValue: undefined,
     _currentValue2: undefined,
     Provider: { '$$typeof': Symbol(react.provider), _context: [Circular] },
     Consumer: [Circular],
     displayName: 'GraphQLContext',
     currentValue:
      GraphQL {
        reset: [Function],
        request: [Function],
        query: [Function],
        cache: {},
        requests: {},
        on: [Function: on],
        off: [Function: off],
        emit: [Function: emit] } },
  key: null,
  ref: null,
  props: { children: [Function] },
  _owner: null }

jaydenseric/graphql-react#12

It's midnight now, I give up.

@trueadm

This comment has been minimized.

Contributor

trueadm commented Oct 31, 2018

@jaydenseric Did you have any luck fixing your issue? I wonder if the follow up PR fixes the issues your experienced: #14033.

@jaydenseric

This comment has been minimized.

jaydenseric commented Nov 1, 2018

@trueadm I did eventually get it working, there were a few things that had to change; here is a diff.

Looking over it a now, a few days later it's hard to remember exactly the issues…

  1. You can’t do if (element.type.Consumer) to check if an element is a context consumer anymore. You have to check the element.type.$$typeof Symbol. The bundle impact is way too big to import it the "proper" way from react-is, so I defined it manually. Also babel/babel#7998 would cause a CJS runtime error.
  2. You can’t do element.type.currentValue to get the current value anymore. You have to do element.type._currentValue.
  3. For some reason, context value could not be retrieved anymore for a context consumer that is a descendant of another context consumer. For a while I have been putting of fixing an edge case bug in graphql-react that was recently fixed in react-apollo, where you have to use maps to create scopes for context under providers. Actioning that fix also worked around the new limitation.
  4. I got really stuck until figuring out that there are 2 ways you need to retrieve a context consumers' provider; 1 each for NODE_ENV undefined (element.type._context.Provider) and production (element.type.Provider). I now run all tests 4 times; CJS dev/prod, ESM dev/prod.

I live in fear non semver major React releases will break this logic again; It's happened twice now. Do you think the changes coming in #14033 will cause this implementation to break again? It's kinda hard for me to tell looking at it.

react-apollo is looking at give up on efficiently walking the tree in favor of multiple renders to string (apollographql/react-apollo#2533) until there is an elegant way to do things the Suspense way. Although that's one of the ways I first considered doing SSR in graphql-react and have recent promising experiments, it might be easier to skip that trouble and avoid React 16.7+ hooks, etc. if it breaks SSR and wait for Suspense. Everyone would like to know, but is it far off?

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