Skip to content

Debug: Print error on changing dependency array length between renders#2732

Open
marvinhagemeister wants to merge 2 commits into
mainfrom
hooks-dep-warning
Open

Debug: Print error on changing dependency array length between renders#2732
marvinhagemeister wants to merge 2 commits into
mainfrom
hooks-dep-warning

Conversation

@marvinhagemeister

Copy link
Copy Markdown
Member

This PR adds a new error log to preact/debug that comes into play when the dependency array length of a hook changes between renders.

Fixes #2728 .

@github-actions

github-actions Bot commented Aug 30, 2020

Copy link
Copy Markdown

Size Change: +563 B (1%)

Total Size: 41 kB

Filename Size Change
debug/dist/debug.js 3.27 kB +176 B (5%) 🔍
debug/dist/debug.module.js 3.26 kB +173 B (5%) 🔍
debug/dist/debug.umd.js 3.36 kB +175 B (5%) 🔍
hooks/dist/hooks.js 1.11 kB +11 B (0%)
hooks/dist/hooks.module.js 1.13 kB +10 B (0%)
hooks/dist/hooks.umd.js 1.19 kB +18 B (1%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.26 kB 0 B
compat/dist/compat.module.js 3.29 kB 0 B
compat/dist/compat.umd.js 3.33 kB 0 B
devtools/dist/devtools.js 185 B 0 B
devtools/dist/devtools.module.js 195 B 0 B
devtools/dist/devtools.umd.js 261 B 0 B
dist/preact.js 3.91 kB 0 B
dist/preact.min.js 3.94 kB 0 B
dist/preact.module.js 3.94 kB 0 B
dist/preact.umd.js 3.98 kB 0 B
test-utils/dist/testUtils.js 437 B 0 B
test-utils/dist/testUtils.module.js 439 B 0 B
test-utils/dist/testUtils.umd.js 515 B 0 B

compressed-size-action

Comment thread hooks/src/index.js
function argsChanged(oldArgs, newArgs) {
return !oldArgs || oldArgs.length !== newArgs.length || newArgs.some((arg, index) => arg !== oldArgs[index]);
function argsChanged(oldArgs, newArgs, currentComponent) {
if (options._argsChanged) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it make any difference if we extend the _hook option to handle this case? Perhaps something like getHookState(currentIndex++, 7, args) (we probably only need to pass args if the hook has an args param)

Looking at it now, should we invoke that _hook option after we have created/retrieved the hook state so we can pass it to the hook? Sounds like that would make the _hook option a lot more useful/powerful.

Will make the debug code more complex but if it saves bytes in product code, maybe that's worth it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just did that with the latest commit, but it turns out to be bigger than expected. It's 15B larger compared to the additional option hook. Another argument in favor of keeping options._hook as is, is that it's a breaking change, even though it's internals. Both devtools and prefresh are built on it.

@github-actions

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 99.615% when pulling 0f6da63 on hooks-dep-warning into bf3f96f on master.

@github-actions

github-actions Bot commented Oct 5, 2020

Copy link
Copy Markdown

📊 Tachometer Benchmark Results

Summary

A summary of the benchmark results will show here once they finish.

Results

The full results of your benchmarks will show here once they finish.

tachometer-reporter-action v2 for CI

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.

useMemo doesn't correctly detect deps changes

3 participants