Separate debug tests into multiple files and fix some bugs in debug #2047
Conversation
The previous test passed because the diffed option was deleted before each test, which deleted the hook.diffed option. In diffed, the hook code flushes useLayoutEffect so in a real app, the debug code would always see any layout effects because it ran after they were run. Because the old test had deleted the hooks option, this flushing code never ran and the debug option always saw the layoutEffects
| @@ -19,7 +21,11 @@ export function initDebug() { | |||
| let oldCatchError = options._catchError; | |||
| let oldRoot = options._root; | |||
| let oldHook = options._hook; | |||
| const warnedComponents = { useEffect: {}, useLayoutEffect: {}, lazyPropTypes: {} }; | |||
| const warnedComponents = !isWeakMapSupported ? null : ({ | |||
There was a problem hiding this comment.
Previously, we used an object to store a map of component instances to booleans indicating if we had warned on them already. Setting a function as a property on an object calls .toString on the function and uses that as the key. This behavior doesn't work if multiple instances of the same constructor function have warnings cuz each instance uses will have the same constructor function string value.
The previous tests passed because we cleared and reset the options before each tests meaning these object got reset for each test. However, in the new separated tests, debug is only initialized once, and the lazy prop types tests started failing cuz all Lazy components share the same constructor function and so different instances of lazy in different tests wouldn't warn when we expected them to.
Using a WeakMap fixes this problem since each component instance (regardless of whether they share the same constructor function) now has its own key/value pair in the WeakMap.
IE11 has basic support for WeakMaps so I think its okay to use them in debug.
| @@ -259,6 +263,8 @@ export function initDebug() { | |||
| } | |||
| } | |||
|
|
|||
| if (oldDiffed) oldDiffed(vnode); | |||
There was a problem hiding this comment.
Hooks use the options.diffed to flush layoutEffects. Debug needs to run its layout effect checks before the hooks option resets them to an empty array, so I moved the calling of other diffed options to after the layout effects checks are run.
This test passed previously because before each test we would delete the diffed option which deleted the hooks option and we never set it back. So the diffed hook option never ran in the old debug tests. Now it does in the new debug-hooks.test.js file.
There was a problem hiding this comment.
Oh that's a good catch 👍
| // an error because the diff saw the oldVNode and the newVNode as having | ||
| // the same ref ("a") and so it didn't try to apply it. It had nothing to do | ||
| // with the "$$typeof" property being present which the old test seems to imply | ||
| it.skip('should not print an error on string refs', () => { |
There was a problem hiding this comment.
Preact core doesn't support string refs and compat doesn't add support for it. No one has complained yet so should we leave this feature out for now and only worry about bringing it back if someone needs it?
There was a problem hiding this comment.
We deprecated string refs with X. I was the one who removed it :D We got 2 users who noticed so far, but they could easily update by switching to a function callback or the ref object. Therefore I think it's safe to remove this test 👍
Previously the debug tests attempted to reset the options object before each test (I think at my suggestion originally lol) to reset the debug state before each test. However, by deleting the options properties, it deleted the hooks options and compat options. It also didn't delete every option so some options would continue to work. This behavior lead to tests that weren't resilient to changes and didn't capture bugs.
To improve our debug tests, I separated the tests out into different files. Each test file only initializes the debug options once. This behavior better mimics what our users do making our tests more closely match our user's behaviors.
Further, some debug behavior changes based on which modules you import (e.g. if you use compat we don't warn on string refs). So separating out the debug tests into different files (one test file imports compat, another doesn't) allows us to simulate these differing behaviors more reliable.
If nothing else, the
debug.test.jsfile was pretty big so making it a little smaller is kinda nice :)This PR also fixes a bug where if different instances of the same component wouldn't print warnings since they share the same constructor (more details in the comments of the PR).