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

Update SlotFillProvider to the new context API #11123

Merged
merged 9 commits into from Nov 2, 2018

Conversation

Projects
None yet
4 participants
@nosolosw
Member

nosolosw commented Oct 26, 2018

To be merged after #11314 lands.

The SlotFillProvider is using the React's legacy context API which will be removed in the next major version and was added as a deprecation in StrictMode in 16.6.0.

This PR updates the SlotFillProvider to the stable React's context API.

Test

  • The popover component works as expected. For example, use the / inserter to add blocks.
  • Install a plugin that uses the SlotFill API (for example: DropIt or Yoast SEO) and make sure it works as expected.

@nosolosw nosolosw self-assigned this Oct 26, 2018

@nosolosw nosolosw requested a review from WordPress/gutenberg-core Oct 26, 2018

@nosolosw nosolosw added this to the 4.2 milestone Oct 26, 2018

@noisysocks

This comment has been minimized.

Member

noisysocks commented Oct 29, 2018

I'm thinking we should upgrade to React v16.6 first so that we can use Class.contextType to migrate these components to the new Context API without having to introduce new wrapper components.

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 29, 2018

I'm thinking we should upgrade to React v16.6 first so that we can use Class.contextType to migrate these components to the new Context API without having to introduce new wrapper components.

I'd rather have this upgrade in 5.0. Using legacy APIs inside Gutenberg justifies its use within third-party (plugins, etc), which are a lot harder to migrate. Also, when we upgrade React we might end doing things differently (hooks anyone? where Class.contextType can't be used), or perhaps at that point there is an even newer/better API built on top of the low-level primitives.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 29, 2018

Moving out of 4.2 as it doesn't really touch the public API but still good to have for 5.0 among other react deprecations/upgrade.

@youknowriad youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 29, 2018

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 29, 2018

Wanted to note that this PR is blocked until the tests are fixed.

The reason why they fail is that we are using an old version of enzyme that doesn't support the new React Context API. Updating enzyme and enzyme-adapter-react-16 fixes the issue. I'll put together a PR to update those dependencies so we can explore their impact. Note that enzyme not fully supporting React 16 has been a problem in the past that forced us to either skip tests, remove, or migrate them to react-test-renderer.

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 29, 2018

Update: I'm going to explore update those tests to the react-test-renderer.

Also, I've created #11214 to keep track of workarounds we do to make (the versions we use of) enzyme and enzyme-adapter-react-16 work with React.

@nosolosw nosolosw force-pushed the update/slotfill-context-api branch from 875b034 to 4c4a70e Oct 30, 2018

<div>
1
</div>,
<button

This comment has been minimized.

@nosolosw

nosolosw Oct 30, 2018

Member

Note that, previously, we weren't doing snapshot testing and the button component wasn't checked for.

<div>
2
</div>,
<button

This comment has been minimized.

@nosolosw

nosolosw Oct 30, 2018

Member

Note that, previously, we weren't doing snapshot testing and the button component wasn't checked for.

first
second
</div>,
<button

This comment has been minimized.

@nosolosw

nosolosw Oct 30, 2018

Member

Note that, previously, we weren't doing snapshot testing and the button component wasn't checked for.

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 30, 2018

Tests updated - note that tests are migrated in atomic commits one by one so it's easier to compare the changes. This is ready for a second review.

@nosolosw nosolosw requested a review from WordPress/gutenberg-core Oct 30, 2018

@nosolosw nosolosw removed the request for review from WordPress/gutenberg-core Oct 30, 2018

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 30, 2018

mmm, some other tests still need to be migrated:

  • Tooltip
  • Popover
  • PluginPostPublishPanel
  • PluginPrePublishPanel
  • PluginPostStatusInfo
@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 31, 2018

Tests have been fixed. This is ready for review.

@gziolo

It would be nice to move all test updates to their own PR. Ensure it works with master and then upgrade Slot and Fill to the new Context API. Otherwise, it's not that obvious if we didn't break anything.

I also would love to avoid using snapshots for Slot tests as they become very hard to reason after changes introduced. In general they are awesome for UI components, but Slot is more container which doesn't care as much about UI but to ensure that all fills are properly rendered.

Show resolved Hide resolved packages/components/src/slot-fill/context.js
Show resolved Hide resolved packages/components/src/slot-fill/index.js Outdated
Show resolved Hide resolved packages/components/src/slot-fill/slot.js Outdated
Show resolved Hide resolved packages/components/src/slot-fill/test/slot.js Outdated

@nosolosw nosolosw force-pushed the update/slotfill-context-api branch from ccdf0bb to d04fb12 Oct 31, 2018

@nosolosw nosolosw changed the base branch from master to update/tests-for-context-api Oct 31, 2018

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 31, 2018

@gziolo Tests are now in a separate PR at #11314 I've rebased this with that PR, so we can merge cleanly after that lands into master.

@gziolo

gziolo approved these changes Oct 31, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 31, 2018

Nice work, changes in this PR looks great. There are a few e2e tests which ensure that plugins can extend editor, so I think it also covers Slot and FIll logic indirectly.

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 31, 2018

Cool! The base branch is #11314 I'll wait until that's approved and merged, then I'll rebase this with master and will merge.

@youknowriad youknowriad referenced this pull request Nov 1, 2018

Open

Upgrade to React 16.6 stable APIs #11360

9 of 11 tasks complete

@nosolosw nosolosw force-pushed the update/tests-for-context-api branch from 6ac3037 to 4e5b61c Nov 2, 2018

@nosolosw nosolosw force-pushed the update/slotfill-context-api branch from cb19a4e to cc36d71 Nov 2, 2018

@youknowriad youknowriad modified the milestones: WordPress 5.0, 4.3 Nov 2, 2018

@nosolosw nosolosw force-pushed the update/slotfill-context-api branch from cc36d71 to 08daa84 Nov 2, 2018

@nosolosw nosolosw changed the base branch from update/tests-for-context-api to master Nov 2, 2018

@nosolosw nosolosw force-pushed the update/slotfill-context-api branch from 08daa84 to 24a9729 Nov 2, 2018

@nosolosw nosolosw merged commit aad371d into master Nov 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nosolosw nosolosw deleted the update/slotfill-context-api branch Nov 2, 2018

antpb added a commit to antpb/gutenberg that referenced this pull request Nov 5, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

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