Support getChildContext() in shallow render #1971

minznerjosh · 2019-01-07T23:26:30Z

Fixes #664

This PR adds support for getChildContext() when shallow-rendering. I've decided to only implement parent-based context, not owner-based context (as I'm not exactly sure what that behavior is, tbh) which means I've excluded react@0.13 from this implementation. (Hope that's okay!)

In react@<=15, the react shallow renderer actually calls getChildContext() for us! This means, as is the case with shouldComponentUpdate(), we just spy on that method's return value.

In react@>=16, getChildContext() is not called (understandable—it's a deprecated API), so we have to call it ourselves.

P.S. Sorry the diff is a little ugly. I ended up messing with the nesting of some conditionals to avoid duplicated checks.

…Context` lifecycle method

minznerjosh · 2019-01-08T19:34:51Z

P.S. I'm pretty sure the build will succeed once is #1968 is in.

ljharb · 2019-01-14T19:37:06Z

@minznerjosh I've rebased this and split up the commits a bit more atomically; it'd be great if you could add some tests to each commit/change :-)

minznerjosh · 2019-01-14T19:43:04Z

@ljharb you’ve got it! I’ll take care of it when I get back from vacation on Wednesday. Thanks for taking a look at this!

minznerjosh · 2019-01-16T17:21:57Z

@ljharb I added tests to every commit except [enzyme-adapter-react-*] [new] add `legacyContextMode`, and `getChildContext` lifecycle method (which is just a configuration change.) This is ready for another round of review!

packages/enzyme/src/ShallowWrapper.js

ljharb · 2019-01-17T08:05:03Z

packages/enzyme/src/ShallowWrapper.js

+  }
+  const childContext = instance.getChildContext();
+  Object.keys(childContext).forEach((key) => {
+    if (!(key in Component.childContextTypes)) {


are we using in here because React does? it seems like this should be an own property check.

Yeah I tried to match react as much as possible and they are using in.

Let's add a comment then, above line 193, linking to https://github.com/facebook/react/blob/1454a8be03794f5e0b23a7e7696cbbbdcf8b0f5d/packages/react-dom/src/server/ReactPartialRenderer.js#L630-L637

packages/enzyme/src/ShallowWrapper.js

minznerjosh · 2019-01-17T16:09:15Z

@ljharb

Fixed the formatting of that ternary
Added tests that simulate an old adapter
Made it so that the context option overrides a parent's context (and added a test)

This is ready for another round of review!

packages/enzyme/src/ShallowWrapper.js

minznerjosh · 2019-01-18T00:07:17Z

@ljharb

Should we implement checkPropTypes in all the adapters, even though it's only used in the >=16 adapters?
Removed multi-line strings
Added comment to explain warning if childContextTypes is not defined, with link to react src
Added comment to explain checking of propTypes implementation, with link to react src
Adjusted implementation of dive() so that options.context (passed to dive()) overrides anything else.

This is ready for another round!

ljharb · 2019-01-19T05:01:59Z

@minznerjosh is there any chance the functionality is actually accessible inside one of the react packages, without copy/paste?

minznerjosh · 2019-01-19T13:03:53Z

@ljharb Unfortunately, I don't think so. 🙁Both pieces of logic live in the react-reconciler package here and here, but neither are public exports, and the public exports that use those functions (like updateContainer) do too much to be useful for us.

minznerjosh · 2019-01-19T18:12:19Z

@ljharb Can we live with the duplication of react logic here? If so, anything else blocking?

ljharb · 2019-01-19T18:12:54Z

It's not ideal, but we can live with it :-) I'm reviewing and making some tweaks locally now.

minznerjosh · 2019-01-19T18:26:21Z

Thank you!! Lemme know if there’s anything else I can do to help.

ljharb · 2019-01-22T07:35:16Z

Thanks for fixing this long-standing bug!

- [new] add `legacyContextMode`, and `getChildContext` lifecycle method (#1971) - [deps] update `enzyme-adapter-utils`, `object.values`, `react-is`, `enzyme-adapter-react-helper` - [dev deps] update `eslint`, `eslint-plugin-react`, `rimraf`, `eslint-plugin-import`, `eslint-plugin-jsx-a11y`

- [new] add `checkPropTypes` (#1971) - [new] add `legacyContextMode`, and `getChildContext` lifecycle method (#1971) - [fix] Filter legacy context on childContextTypes when necessary (#1968) - [deps] update `enzyme-adapter-utils`, `react-is` - [dev deps] update `eslint-plugin-import`, `eslint`, `eslint-plugin-react`, `rimraf`, `eslint-plugin-react`, `eslint-plugin-jsx-a11y`

- [new] add `checkPropTypes` (#1971) - [new] add `legacyContextMode`, and `getChildContext` lifecycle method (#1971) - [fix] Filter legacy context on childContextTypes when necessary (#1968) - [deps] update `enzyme-adapter-utils`, `react-is`, `object.values` - [dev deps] update `eslint-plugin-import`, `eslint`, `eslint-plugin-react`, `rimraf`, `eslint-plugin-react`, `eslint-plugin-jsx-a11y`

minznerjosh added 4 commits January 7, 2019 18:20

[enzyme-adapter-utils] [new] expose getComponentStack

db090c6

[enzyme-adapter-react-*] [new] add legacyContextMode, and `getChild…

3ecf054

…Context` lifecycle method

[enzyme-adapter-react-{16,16.1,16.2,16.3}] [new] add checkPropTypes

8294b79

[new] shallow: Support getChildContext()

02d6337

minznerjosh force-pushed the josh__shallow-get-child-context branch 4 times, most recently from a301c92 to 56d78bf Compare January 8, 2019 19:33

minznerjosh force-pushed the josh__shallow-get-child-context branch from 56d78bf to 5a4f92b Compare January 8, 2019 23:39

ljharb force-pushed the josh__shallow-get-child-context branch from 5a4f92b to f0cd7e2 Compare January 14, 2019 19:36

ljharb added this to In progress in context interaction via automation Jan 14, 2019

minznerjosh force-pushed the josh__shallow-get-child-context branch 4 times, most recently from f426448 to ddf3550 Compare January 16, 2019 17:20

ljharb reviewed Jan 17, 2019

View reviewed changes

ljharb force-pushed the josh__shallow-get-child-context branch from ddf3550 to cc59eab Compare January 17, 2019 08:18

minznerjosh force-pushed the josh__shallow-get-child-context branch from cc59eab to 47b860f Compare January 17, 2019 16:07

ljharb reviewed Jan 17, 2019

View reviewed changes

packages/enzyme/src/ShallowWrapper.js Outdated Show resolved Hide resolved

packages/enzyme/src/ShallowWrapper.js Outdated Show resolved Hide resolved

packages/enzyme/src/ShallowWrapper.js Outdated Show resolved Hide resolved

minznerjosh force-pushed the josh__shallow-get-child-context branch from 47b860f to f12f0dc Compare January 17, 2019 23:58

This comment has been minimized.

Sign in to view

ljharb force-pushed the josh__shallow-get-child-context branch 2 times, most recently from 9fba55d to 02d6337 Compare January 21, 2019 22:44

ljharb merged commit 02d6337 into enzymejs:master Jan 22, 2019

context interaction automation moved this from In progress to Done / Closed Jan 22, 2019

minznerjosh deleted the josh__shallow-get-child-context branch January 22, 2019 17:45

ljharb added this to Context in React 16 Jan 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support getChildContext() in shallow render #1971

Support getChildContext() in shallow render #1971

minznerjosh commented Jan 7, 2019

minznerjosh commented Jan 8, 2019

ljharb commented Jan 14, 2019

minznerjosh commented Jan 14, 2019

minznerjosh commented Jan 16, 2019

ljharb Jan 17, 2019

minznerjosh Jan 17, 2019

ljharb Jan 17, 2019

minznerjosh commented Jan 17, 2019

minznerjosh commented Jan 18, 2019

ljharb commented Jan 19, 2019

minznerjosh commented Jan 19, 2019

This comment has been minimized.

minznerjosh commented Jan 19, 2019

ljharb commented Jan 19, 2019

minznerjosh commented Jan 19, 2019

ljharb commented Jan 22, 2019

Support getChildContext() in shallow render #1971

Support getChildContext() in shallow render #1971

Conversation

minznerjosh commented Jan 7, 2019

minznerjosh commented Jan 8, 2019

ljharb commented Jan 14, 2019

minznerjosh commented Jan 14, 2019

minznerjosh commented Jan 16, 2019

ljharb Jan 17, 2019

Choose a reason for hiding this comment

minznerjosh Jan 17, 2019

Choose a reason for hiding this comment

ljharb Jan 17, 2019

Choose a reason for hiding this comment

minznerjosh commented Jan 17, 2019

minznerjosh commented Jan 18, 2019

ljharb commented Jan 19, 2019

minznerjosh commented Jan 19, 2019

This comment has been minimized.

minznerjosh commented Jan 19, 2019

ljharb commented Jan 19, 2019

minznerjosh commented Jan 19, 2019

ljharb commented Jan 22, 2019