Skip to content
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

Multiple state hooks in a component #3645

Merged
merged 3 commits into from
Aug 5, 2022
Merged

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Aug 4, 2022

Currently the last sCU wins which basically means we are losing a bit of granularity, as well as filtering out React.memo when wrapped. The new flow looks like the following:

  • hold a flag whether we have added SCU
  • when we traverse if no state-hooks have a pending next state --> update because this means no state setters have been invoked for this component (call former SCU)
  • when there are invocations look at whether all of the ones with a nextState have equal states
    • if they all have equal states between next and original bail out
    • if not continue updating (call former SCU)

@JoviDeCroock JoviDeCroock changed the title Multiple state hooks Multiple state hooks in a component Aug 4, 2022
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 99.464% when pulling a356999 on multiple-state-hooks into f4bd535 on master.

@github-actions
Copy link

github-actions bot commented Aug 4, 2022

Size Change: +180 B (0%)

Total Size: 43.7 kB

Filename Size Change
hooks/dist/hooks.js 1.38 kB +63 B (4%)
hooks/dist/hooks.module.js 1.4 kB +57 B (4%)
hooks/dist/hooks.umd.js 1.46 kB +60 B (4%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.67 kB 0 B
compat/dist/compat.module.js 3.65 kB 0 B
compat/dist/compat.umd.js 3.73 kB 0 B
debug/dist/debug.js 3 kB 0 B
debug/dist/debug.module.js 3 kB 0 B
debug/dist/debug.umd.js 3.08 kB 0 B
devtools/dist/devtools.js 230 B 0 B
devtools/dist/devtools.module.js 238 B 0 B
devtools/dist/devtools.umd.js 305 B 0 B
dist/preact.js 4 kB 0 B
dist/preact.min.js 4.03 kB 0 B
dist/preact.module.js 4.02 kB 0 B
dist/preact.umd.js 4.06 kB 0 B
jsx-runtime/dist/jsxRuntime.js 317 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 327 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 395 B 0 B
test-utils/dist/testUtils.js 444 B 0 B
test-utils/dist/testUtils.module.js 444 B 0 B
test-utils/dist/testUtils.umd.js 521 B 0 B

compressed-size-action

@JoviDeCroock JoviDeCroock marked this pull request as ready for review August 4, 2022 19:27
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

As already mentioned in slack: Really impressed with your debugging skills on this one! This is great 💯

hooks/src/index.js Show resolved Hide resolved
// we bail out of updating.
// Thinking: would this be dangerous with a batch of updates where
// Comp1 updates --> Comp2 updated in same batch twice but has same eventual state --> this leads to us
// not diving into Comp3
Copy link
Member

Choose a reason for hiding this comment

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

Right now I can't come up with a scenario where that would break. We're already somewhat in edge case territory here with components being re-scheduled as part of the same commit. So I'd say let's keep on eye on that, but I don't expect there to be issues.

// have values that aren't equal to one another this pushes
// us to update further down the tree
const shouldSkipUpdating = stateHooks.every(hookItem => {
if (!hookItem._nextValue) return true;
Copy link
Member

Choose a reason for hiding this comment

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

Per Marvin's comment above, shouldn't this check for _nextValue being undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

_nextValue in this case is an array like the normal state hook containing [value, updater] so using ! is the least bytes and correct as it's array or undefined

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.

None yet

4 participants