refactor(context): Update to latest React Context API #391

derrickhnguyen · 2018-08-28T20:37:38Z

Description

The React team will not support the legacy context API in React 17. To keep up-to-date with latest changes and improvements from the React team, we must transition all usage of the legacy context API with the newest one.

How Has This Been Tested?

Unit tests

derrickhnguyen · 2018-08-29T21:25:26Z

src/defaults.js

@@ -60,4 +59,4 @@ export default {
    '2XL': 6,
  },
  verticalGutter: 3, // value (in base units) that separates columns vertically
-}
+}: Object)


I'm conflicted with this. I tried converting this into $NonMaybeType<ConfigContextType> but then withResolution.logic.js complains with a Flow error of: undefined is incompatible with number, which is ironic since i was hoping $NonMaybeType<> would fix that.

Have you tried with $Shape<ConfigContextType> and { ...ConfigContextType }?

I did. I get this error:

Cannot assign object literal to defaults because: • property base is not writable in property base. • property columns is not writable in property columns. • property displayAliases is not writable in property displayAliases. • property fallbackDisplayKey is not writable in property fallbackDisplayKey. • property gutter is not writable in property gutter. • property maxPageWidth is not writable in property maxPageWidth. • property minPageWidth is not writable in property minPageWidth. • property pageMargin is not writable in property pageMargin. • property spacingAliases is not writable in property spacingAliases. • property verticalGutter is not writable in property verticalGutter. 1│ // @flow 2│ import { type ConfigContextType } from './types' 3│ 4│ const defaults: $Shape<ConfigContextType> = { 5│ base: 8, // multiplier (in pixels) that all other size units use 6│ columns: 12, // number of columns used in the layout 7│ displayAliases: { 8│ // aliases used for the different display breakpoints: 9│ small: [ 10│ { 11│ // - "small" alias used when width is less than 600px 12│ maxWidth: '599px', 13│ }, 14│ { 15│ maxDeviceWidth: '599px', 16│ }, 17│ ], 18│ medium: [ : 50│ // aliases used to indicate spacing values (margin/padding) in base 51│ XS: 0.5, // units. 52│ 'S/2': 0.5, 53│ S: 1, 54│ 'M/2': 1, 55│ M: 2, 56│ 'L/2': 1.5, 57│ L: 3, 58│ 'XL/2': 2, 59│ XL: 4, 60│ '2XL/2': 3, 61│ '2XL': 6, 62│ }, 63│ verticalGutter: 3, // value (in base units) that separates columns vertically 64│ } 65│ 66│ export default defaults 67│

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/utils/index.js:193:12 undefined [1] is incompatible with number [2]. src/utils/index.js [2] 185│ gutter: number, : 190│ spacingProps, 191│ base, 192│ spacingAliases, 193│ gutter = defaults.gutter, 194│ verticalGutter = defaults.verticalGutter, 195│ }: CombineSpacingSettings) { 196│ const combinedSpacingAliases = { src/types.js [1] 56│ +gutter?: number, Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/utils/index.js:194:20 undefined [1] is incompatible with number [2]. src/utils/index.js [2] 186│ verticalGutter: number, : 191│ base, 192│ spacingAliases, 193│ gutter = defaults.gutter, 194│ verticalGutter = defaults.verticalGutter, 195│ }: CombineSpacingSettings) { 196│ const combinedSpacingAliases = { 197│ ...defaults.spacingAliases, src/types.js [1] 63│ +verticalGutter?: number, Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/withResolution/withResolution.logic.js:51:24 undefined [1] is incompatible with string [2]. src/withResolution/withResolution.logic.js 48│ props, 49│ shouldShow, 50│ resolutionKeys = [], 51│ fallbackDisplayKey = defaults.fallbackDisplayKey, 52│ }: {| 53│ +props: *, 54│ +shouldShow?: ShouldShow, 55│ +resolutionKeys: Array<string>, [2] 56│ +fallbackDisplayKey: string, src/types.js [1] 55│ +fallbackDisplayKey?: string,

@arahansen do you have other suggestions?

this worked for me:

{| ...ConfigContextType, gutter: number, fallbackDisplayKey: string, verticalGutter: number, |}

…js/gymnast into denguyen/new-context-api

obartra · 2018-09-05T19:54:43Z

scripts/webpack.config.js

@@ -46,8 +46,6 @@ module.exports = {
  devtool: 'source-map',
  externals: {
    react: 'react',
-    'react-dom': 'react-dom',


is react-dom still needed here then?

You're right, I'll add this back.

obartra · 2018-09-05T19:56:24Z

This is a breaking change, make sure the combined commit says "BREAKING CHANGE" on the last line following conventional changelog format (otherwise there won't be a major version increase)

obartra · 2018-09-05T19:59:05Z

src/configContext/index.js

+export default {
+  Provider: ConfigProvider,
+  Consumer: ConfigConsumer,
+}


why export both? We only need ConfigConsumer externally (the provider is only used internally, I would not export it)

Make sense. I'll make Provider only exportable inside it's own file but not in gymnast.js

As I was working on removing this, I noticed (via Storybook) that when using <ConfigProvider> imported from a relative path alongside a component (i.e. <Grid>) imported from a gymnast import, that <Grid> will not use the same React.createContext() as provided from <ConfigProvider> when imported from a relative path.

This is only an issue in Storybook in the Config Provider section. However, this shouldn't be a problem in production, since we're only exporting Consumer, which should be using the same React.createcontext() as the Provider imported from gymnast.

Example:

// @flow // storybook/stories/configProvider/columns.js import * as React from 'react' import { times } from 'lodash' import { number } from '@storybook/addon-knobs' import { Grid } from 'gymnast' import { colors } from '../../shared' // This is using a different `React.createContext()` than the imported // Grid above. Therefore, Grid isn't going to consume the 'columns' that // was passed through from this ConfigProvider, and instead, will used // the default columns value. import ConfigProvider from '../../../src/configContext/configProvider' export default () => { const columns = number('Columns', 10, { range: true, min: 1, max: 24 }) const items = number('Items', 20, { range: true, min: 1, max: 48 }) return ( <ConfigProvider columns={columns}> <Grid padding="0 L/2"> {times(items, key => ( <Grid key={key} padding="0 L/2 L" size={1}> <Grid style={colors.colors1} padding="L/2" align="center" justify="center" > {key + 1} </Grid> </Grid> ))} </Grid> </ConfigProvider> ) }

This appears to be a known issue via this issue

To alleviate the Storybook problem, I can remove all imports from gymnast and just imported via relative path, like so:

// @flow // storybook/stories/configProvider/columns.js import * as React from 'react' import { times } from 'lodash' import { number } from '@storybook/addon-knobs' import { colors } from '../../shared' import ConsumerProvider from '../../../src/configContext/configProvider' import Grid from '../../../src/grid' export default () => { const columns = number('Columns', 10, { range: true, min: 1, max: 24 }) const items = number('Items', 20, { range: true, min: 1, max: 48 }) return ( <ConsumerProvider columns={columns}> <Grid padding="0 L/2"> {times(items, key => ( <Grid key={key} padding="0 L/2 L" size={1}> <Grid style={colors.colors1} padding="L/2" align="center" justify="center" > {key + 1} </Grid> </Grid> ))} </Grid> </ConsumerProvider> ) }

We could add a babel alias to it to make it a bit more convenient to use. If we define one (for stories only) that maps gymnastSrc to the src path we could then do something like gymnastSrc/configContext/configProvider

to ensure the project is still tree-shakeable with babel-plugin-lodash this index.js file needs to contain as default export the same value we export through src/index.js

obartra · 2018-09-05T20:02:21Z

src/configContext/index.spec.js

+          <TesterComponent>{render2}</TesterComponent>
+        </ConfigContext.Provider>
+        <TesterComponent>{render3}</TesterComponent>
+      </ConfigContext.Provider>


👍 nice test cases

obartra · 2018-09-05T20:03:28Z

src/utils/index.js

-  const contextValues = get(context, 'gymnast', {})
-
-  return { ...defaults, ...contextValues, ...overrides }
+  return { ...defaults, ...context, ...overrides }


this method looks so nice now ✨

obartra · 2018-09-06T15:13:43Z

test/e2e/__snapshots__/gymnast.spec.js.snap

@@ -3,7 +3,10 @@
 exports[`e2e tests should export the same values from the main export than individual files 1`] = `
 Object {
  "Col": [Function],
-  "ConfigProvider": [Function],


with the update to not export config context, will we keep the same API?

Once I make my changes, it will look like:

Object { "Col": [Function], "ConfigContext": Object { "Consumer": [Function], },

or another option to create a separate folder for configConsumer, then the snapshot could look like:

Object { "Col": [Function], "ConfigConsumer": [Function],

why not keep the API the same? if we change it we'll have to have a separate v1 vs v0 docs

Wouldn't it be confusing to name it ConfigProvider, but only export a ConfigConsumer?

Internally you see it as Provider / Consumer because we are implementing it using React Context API but the gymnast consumer should not be aware of these details.

Here we are exposing a ConfigProvider for the user to customize gymnast (a Component they can use to provide configuration options).

It's a common pattern (both in naming and behavior) that Redux and Apollo use.

Okay, the way Redux and Apollo are exposing their context is by externally exporting their Provider component while leaving their Consumer component un-exported, but providing a HOC component to access the context value (i.e. connect() in Redux and <Query> in Apollo).

So should your comment via src/configContext/index.js be reversed where we should externally export our Provider and not our Consumer?

whichever one we export here should be the one exported as default on configContext/index.js or the lodash plugin will export the wrong thing

obartra · 2018-09-06T15:15:06Z

src/gymnast.js

@@ -3,7 +3,7 @@ import './cxs'

 // Components
 export { default as Col } from './col'
-export { default as ConfigProvider } from './configProvider'
+export { default as ConfigContext } from './configContext'
 export { default as Grid } from './grid'
 export { default as Layout } from './layout'
 export { default as Offset } from './offset'


should we export withContext for consistency as well?

derrickhnguyen · 2018-09-06T16:50:16Z

src/configProvider/index.js

    }
  }
+
+  state = {}


Decided to keep this here, since getting warning via yarn test:

$ jest -u PASS src/asCol/asCol.spec.js PASS src/asLayout/layout.spec.js PASS src/offset/offset.spec.js PASS src/root/root.spec.js PASS src/dev/dev.spec.js PASS src/withResolution/withResolution.logic.spec.js PASS src/core/core.spec.js PASS src/withResolution/withResolution.spec.js PASS src/log/log.spec.js PASS src/withResolution/mediaQuery.spec.js PASS src/errors/index.spec.js PASS src/utils/utils.spec.js PASS src/asGrid/grid.spec.js PASS src/configProvider/index.spec.js ● Console console.error node_modules/fbjs/lib/warning.js:33 Warning: ConfigProvider: Did not properly initialize state during construction. Expected state to be an object, but it was undefined. PASS test/e2e/gymnast.spec.js PASS src/withContext/index.spec.js ● Console console.error node_modules/fbjs/lib/warning.js:33 Warning: ConfigProvider: Did not properly initialize state during construction. Expected state to be an object, but it was undefined. Test Suites: 16 passed, 16 total Tests: 3 skipped, 118 passed, 121 total Snapshots: 4 passed, 4 total Time: 4.683s Ran all test suites. ✨ Done in 6.51s.

obartra

This looks perfect, let's make sure the commit message is correct before merging (so it's released correctly) but after that !

🎉

obartra · 2018-09-06T21:30:20Z

🎉 This PR is included in version 16.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Derrick Nguyen added 3 commits August 27, 2018 08:15

Initiated new context api

bb75b23

Initiated new context api

10e6f5f

Fixed merge conflicts.

394f2a7

derrickhnguyen requested review from andrewmtoy, obartra and arahansen August 28, 2018 20:37

Derrick Nguyen and others added 5 commits August 28, 2018 16:07

Refactored code to be more DRY

89bedb8

Removed prop-types as a peer-dependency

17d872c

Removed prop-types as a peer-dependency

0ca9ebe

Fixed merge conflicts.

2d5f4be

Removed 'gymnast' as a property inside context.

ee1fa3a

obartra mentioned this pull request Aug 29, 2018

refactor(context): Switch to new context API #368

Closed

3 tasks

Derrick Nguyen added 3 commits August 29, 2018 13:43

Made defaults read-only.

75d5fda

Made defaults read-only.

074ece6

Fixed merge conflicts.

4623a5b

derrickhnguyen commented Aug 29, 2018

View reviewed changes

Derrick Nguyen and others added 3 commits August 29, 2018 20:03

Fixed merge conflicts.

246da50

Merge branch 'denguyen/new-context-api' of https://github.com/gymnast…

20ca746

…js/gymnast into denguyen/new-context-api

Replaced omit function with spread operation

5b44972

obartra reviewed Sep 5, 2018

View reviewed changes

Added react-dom back as a peer-dependency and refactor type for defaults

14e1a40

obartra reviewed Sep 6, 2018

View reviewed changes

Final refactoring.

19e1eb4

derrickhnguyen commented Sep 6, 2018

View reviewed changes

obartra approved these changes Sep 6, 2018

View reviewed changes

arahansen approved these changes Sep 6, 2018

View reviewed changes

Changed peerDependency so that gymnast now requires React@16.3

6e0d937

derrickhnguyen merged commit 1d31921 into master Sep 6, 2018

derrickhnguyen deleted the denguyen/new-context-api branch September 6, 2018 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(context): Update to latest React Context API #391

refactor(context): Update to latest React Context API #391

derrickhnguyen commented Aug 28, 2018 •

edited

Loading

derrickhnguyen Aug 29, 2018 •

edited

Loading

obartra Sep 5, 2018

derrickhnguyen Sep 5, 2018

obartra Sep 5, 2018

arahansen Sep 5, 2018

obartra Sep 5, 2018

derrickhnguyen Sep 5, 2018

obartra commented Sep 5, 2018

obartra Sep 5, 2018

derrickhnguyen Sep 5, 2018

derrickhnguyen Sep 6, 2018

obartra Sep 6, 2018

obartra Sep 6, 2018

obartra Sep 5, 2018

obartra Sep 5, 2018

obartra Sep 6, 2018

derrickhnguyen Sep 6, 2018

obartra Sep 6, 2018

derrickhnguyen Sep 6, 2018 •

edited

Loading

obartra Sep 6, 2018

derrickhnguyen Sep 6, 2018

obartra Sep 6, 2018

obartra Sep 6, 2018

derrickhnguyen Sep 6, 2018

obartra left a comment

obartra commented Sep 6, 2018

refactor(context): Update to latest React Context API #391

refactor(context): Update to latest React Context API #391

Conversation

derrickhnguyen commented Aug 28, 2018 • edited Loading

Description

How Has This Been Tested?

derrickhnguyen Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obartra commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrickhnguyen Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obartra left a comment

Choose a reason for hiding this comment

obartra commented Sep 6, 2018

derrickhnguyen commented Aug 28, 2018 •

edited

Loading

derrickhnguyen Aug 29, 2018 •

edited

Loading

derrickhnguyen Sep 6, 2018 •

edited

Loading