Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Exploration of hooks and context #2206

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Exploration of hooks and context #2206

wants to merge 4 commits into from

Conversation

smashwilson
Copy link
Contributor

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

My goal here is to investigate using React's useContext hook to address our prop drilling problem (#1437). The major reason why useContext is suitable as a potential solution here is that it makes consuming multiple context objects within a single component ergonomic:

export default function SomeComponent(props) {
  const atomEnv = useContext(AtomContext);
  const repositoryEnv = useContext(RepositoryContext);

  // ...
}

Doing this with traditional contexts meant a layer of render prop per context:

export default class SomeComponent() {
  render() {
    return (
      <AtomContext.Consumer>
        {atomEnv => (
          <RepositoryContext.Consumer>
             {repository => {
               // ...
             }}
          </RepositoryContext.Consumer>
        )}
      </AtomContext.Consumer>
    );
  }
}

My plan is to introduce a context package for each major grouping of commonly drilled props, along with utility methods and custom hooks to ease common access patterns.

Screenshot/Gif

N/A

Alternate Designs

See #2027 for my pre-hooks take on the problem.

Benefits

Improved maintainability by not having to pass large sets of Atom-related props through deep component hierarchies.

Possible Drawbacks

useContext only works within functional components, so this basically involves gradually re-writing everything.

The Enzyme shallow renderer has issues with functional components and hooks, so this will need to wait until those are fixed and included in an Enzyme release: enzymejs/enzyme#2176, enzymejs/enzyme#2086.

Applicable Issues

#1437

Metrics

N/A

Tests

N/A

Documentation

N/A

Release Notes

N/A

User Experience Research (Optional)

N/A

@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #2206 into master will decrease coverage by 16.06%.
The diff coverage is 16.78%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2206       +/-   ##
===========================================
- Coverage   92.72%   76.65%   -16.07%     
===========================================
  Files         215      217        +2     
  Lines       12271    12153      -118     
  Branches     1796     1772       -24     
===========================================
- Hits        11378     9316     -2062     
- Misses        893     2837     +1944
Impacted Files Coverage Δ
lib/atom/status-bar.js 0% <0%> (-84.22%) ⬇️
lib/controllers/root-controller.js 0.77% <1.26%> (-82.1%) ⬇️
lib/views/changed-files-count-view.js 100% <100%> (ø) ⬆️
lib/views/clone-dialog.js 10.81% <11.11%> (-81.26%) ⬇️
lib/views/push-pull-view.js 1.72% <2%> (-88.44%) ⬇️
lib/views/branch-menu-view.js 2.38% <2.38%> (-89.46%) ⬇️
lib/views/branch-view.js 25% <25%> (-50%) ⬇️
lib/atom/tooltip.js 8.1% <3.22%> (-88.05%) ⬇️
lib/views/credential-dialog.js 44% <44%> (-49.55%) ⬇️
lib/github-package.js 1.98% <50%> (-64.81%) ⬇️
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02233cb...e67f0ce. Read the comment docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant