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

Show deprecated context object warnings usage in ReactDOM server #14033

Merged
merged 6 commits into from
Nov 7, 2018

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Oct 30, 2018

This is a follow up to #13829.

This PR adds deprecated context object usage warnings that mirror the same warnings as in #13829. Specifically:

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.

This behaviour is backwards compatible with previous versions of React.

@sizebot
Copy link

sizebot commented Oct 30, 2018

Details of bundled changes.

Comparing: 8eca0ef...65cab46

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-server.browser.development.js +1.3% +1.4% 118.4 KB 119.89 KB 31.35 KB 31.8 KB UMD_DEV
react-dom-server.browser.development.js +1.3% +1.5% 114.53 KB 116.02 KB 30.39 KB 30.84 KB NODE_DEV
react-dom-server.node.development.js +1.3% +1.4% 116.45 KB 117.94 KB 30.93 KB 31.37 KB NODE_DEV
ReactDOMServer-dev.js +1.3% +1.6% 114.63 KB 116.17 KB 29.87 KB 30.33 KB FB_WWW_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

@engprodigy
Copy link

I'm trying to follow the changes as a follow up to PR #13829

@trueadm trueadm requested review from bvaughn and acdlite November 5, 2018 11:16
@bvaughn
Copy link
Contributor

bvaughn commented Nov 5, 2018

Looks like you need to run yarn prettier to update packages/react-dom/src/server/ReactPartialRenderer.js btw

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 think this looks okay

// We expect 1 error.
await render(<App />, 1);
},
);
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests seem a little weak since they don't assert on what the error message actually is. I guess ReactDOMServerIntegrationTestUtils doesn't provide a mechanism to do this currently, but it seems like it would be an improvement if e.g. expectErrors supported either a number or an error of partial messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, this is something I stumbled into. It can be something we do as a follow up as I know that we're planning on making some changes to the server renderer and this might flow nicely with that workload.

const consumer: ReactConsumer<any> = (nextChild: any);
const nextProps: any = consumer.props;
const nextValue = consumer.type._currentValue;
let reactContext: ReactContext<any> = (nextChild: any).type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a little odd that we're any-casting something that explicitly might not be ReactContext to that type for DEV mode.

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.

5 participants