Skip to content

Refactor document capture provider hierarchy via component composition#5535

Merged
aduth merged 4 commits intomainfrom
aduth-compose-components
Oct 26, 2021
Merged

Refactor document capture provider hierarchy via component composition#5535
aduth merged 4 commits intomainfrom
aduth-compose-components

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Oct 22, 2021

Why: Flattening results in diffs which aren't as unyieldy when adding or removing from the component hierarchy.

**Why**: Flattening results in diffs which aren't as unyieldy when adding or removing from the component hierarchy.
Comment on lines +142 to +143
const App = composeComponents([
[AppContext.Provider, { value: appContext }],
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 considered a version that preserved JSX, but unfortunately it doesn't always work as nice with component typing which enforces that children be passed. We can control this for our own components, but it's quite likely to be something we'd encounter if ever using third-party components.

  const App = composeComponents([
    <AppContext.Provider value={appContext} />,

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

aduth added 3 commits October 25, 2021 14:58
**Why**: Avoid verbose equivalent TypeScript casting
**Why**: Destructures the same, avoids explicit reference to undefined (which by definition is more on the implicit end "absence of" a defined value, vs. explicit empty null), smaller bundled size.
@aduth aduth merged commit aa3b662 into main Oct 26, 2021
@aduth aduth deleted the aduth-compose-components branch October 26, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants