Separate debug tests into multiple files and fix some bugs in debug #2047
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import { createElement, render } from 'preact/compat'; | ||
| import { initDebug } from '../../src/debug'; | ||
| import { setupScratch, teardown } from '../../../test/_util/helpers'; | ||
|
|
||
| /** @jsx createElement */ | ||
|
|
||
| initDebug(); | ||
|
|
||
| describe('debug with compat', () => { | ||
| let scratch; | ||
| let errors = []; | ||
| let warnings = []; | ||
|
|
||
| beforeEach(() => { | ||
| errors = []; | ||
| warnings = []; | ||
| scratch = setupScratch(); | ||
| sinon.stub(console, 'error').callsFake(e => errors.push(e)); | ||
| sinon.stub(console, 'warn').callsFake(w => warnings.push(w)); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| (console.error).restore(); | ||
| console.warn.restore(); | ||
| teardown(scratch); | ||
| }); | ||
|
|
||
| // TODO: This test only passed before because the old test first rendered | ||
| // a string ref and expected an error. Then it rerendered a vnode with the | ||
| // same string ref and did not expect an error. The rerender didn't throw | ||
| // 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 |
||
| let fn = () => render(<div ref="a" />, scratch); | ||
| expect(fn).to.not.throw(); | ||
| expect(console.error).to.not.be.called; | ||
| }); | ||
| }); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| import { createElement, render, Component } from 'preact'; | ||
| import { | ||
| useState, | ||
| useEffect, | ||
| useLayoutEffect, | ||
| useMemo, | ||
| useCallback | ||
| } from 'preact/hooks'; | ||
| import { initDebug } from '../../src/debug'; | ||
| import { act } from 'preact/test-utils'; | ||
| import { setupScratch, teardown } from '../../../test/_util/helpers'; | ||
|
|
||
| /** @jsx createElement */ | ||
|
|
||
| initDebug(); | ||
|
|
||
| describe('debug with hooks', () => { | ||
| let scratch; | ||
| let errors = []; | ||
| let warnings = []; | ||
|
|
||
| beforeEach(() => { | ||
| errors = []; | ||
| warnings = []; | ||
| scratch = setupScratch(); | ||
| sinon.stub(console, 'error').callsFake(e => errors.push(e)); | ||
| sinon.stub(console, 'warn').callsFake(w => warnings.push(w)); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| (console.error).restore(); | ||
| console.warn.restore(); | ||
| teardown(scratch); | ||
| }); | ||
|
|
||
| // TODO: Fix this test. It only passed before because App was the first component | ||
| // into render so currentComponent in hooks/index.js wasn't set yet. However, | ||
| // any children under App wouldn't have thrown the error if they did what App | ||
| // did because currentComponent would be set to App. | ||
| // In other words, hooks never clear currentComponent so once it is set, it won't | ||
| // be unset | ||
| it.skip('should throw an error when using a hook outside a render', () => { | ||
| const Foo = props => props.children; | ||
| class App extends Component { | ||
| componentWillMount() { | ||
| useState(); | ||
| } | ||
|
|
||
| render() { | ||
| return <p>test</p>; | ||
| } | ||
| } | ||
| const fn = () => | ||
| act(() => | ||
| render( | ||
| <Foo> | ||
| <App /> | ||
| </Foo>, | ||
| scratch | ||
| ) | ||
| ); | ||
| expect(fn).to.throw(/Hook can only be invoked from render/); | ||
| }); | ||
|
|
||
| // TODO: Fix this test. It only passed before because render was never called. | ||
| // Once render is called, currentComponent is set and never unset so calls to | ||
| // hooks outside of components would still work. | ||
| it.skip('should throw an error when invoked outside of a component', () => { | ||
| function Foo(props) { | ||
| useEffect(() => {}); // Pretend to use a hook | ||
| return props.children; | ||
| } | ||
|
|
||
| const fn = () => | ||
| act(() => { | ||
| render(<Foo>Hello!</Foo>, scratch); | ||
| useState(); | ||
| }); | ||
| expect(fn).to.throw(/Hook can only be invoked from render/); | ||
| }); | ||
|
|
||
| it('should warn for argumentless useEffect hooks', () => { | ||
| const App = () => { | ||
| const [state] = useState('test'); | ||
| useEffect(() => 'test'); | ||
| return <p>{state}</p>; | ||
| }; | ||
| render(<App />, scratch); | ||
| expect(warnings[0]).to.match(/You should provide an array of arguments/); | ||
| render(<App />, scratch); | ||
| expect(warnings[1]).to.be.undefined; | ||
| }); | ||
|
|
||
| it('should warn for argumentless useLayoutEffect hooks', () => { | ||
| const App = () => { | ||
| const [state] = useState('test'); | ||
| useLayoutEffect(() => 'test'); | ||
| return <p>{state}</p>; | ||
| }; | ||
| render(<App />, scratch); | ||
| expect(warnings[0]).to.match(/You should provide an array of arguments/); | ||
| render(<App />, scratch); | ||
| expect(warnings[1]).to.be.undefined; | ||
| }); | ||
|
|
||
| it('should not warn for argumented effect hooks', () => { | ||
| const App = () => { | ||
| const [state] = useState('test'); | ||
| useLayoutEffect(() => 'test', []); | ||
| useEffect(() => 'test', [state]); | ||
| return <p>{state}</p>; | ||
| }; | ||
| const fn = () => act(() => render(<App />, scratch)); | ||
| expect(fn).to.not.throw(); | ||
| }); | ||
|
|
||
| it('should warn for useless useMemo calls', () => { | ||
| const App = () => { | ||
| const [people] = useState([40, 20, 60, 80]); | ||
| const retiredPeople = useMemo(() => people.filter(x => x >= 60)); | ||
| const cb = useCallback(() => () => 'test'); | ||
| return <p onClick={cb}>{retiredPeople.map(x => x)}</p>; | ||
| }; | ||
| render(<App />, scratch); | ||
| expect(warnings.length).to.equal(2); | ||
| }); | ||
|
|
||
| it('should warn when non-array args is passed', () => { | ||
| const App = () => { | ||
| const foo = useMemo(() => 'foo', 12); | ||
| return <p>{foo}</p>; | ||
| }; | ||
| render(<App />, scratch); | ||
| expect(warnings[0]).to.match(/without passing arguments/); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.toStringon 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.