Refactor/test connect #1781
Refactor/test connect #1781
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c9a5b8f:
|
|
✔️ Deploy Preview for react-redux-docs ready! 🔨 Explore the source changes: c9a5b8f 🔍 Inspect the deploy log: https://app.netlify.com/sites/react-redux-docs/deploys/61059528d5c74e0007adda45 😎 Browse the preview: https://deploy-preview-1781--react-redux-docs.netlify.app |
|
Looks okay so far. Lemme know when you have the rest of the logic uncommented and converted! |
|
@markerikson Because it's a big file, I think it's a good idea for you to check it earlier, and comment out the test cases that are not converted, because they don't pass the check and all of them are temporarily commented out until I convert them little by little, Finally, thank you for giving me valuable writing instructions |
ok, thanks |
describe: Performance optimizations and bail-outs it: should not attempt to set state when dispatching in componentWillUnmount should not attempt to set state after unmounting should allow to clean up child state in parent componentWillUnmount
myNameIsDu
force-pushed
the
refactor/test-connect
branch
from
07e0f4e
to
545f6f9
Compare
describe: Wrapped component and HOC handling Store subscriptions and nesting add a package: @types/create-react-class
myNameIsDu
force-pushed
the
refactor/test-connect
branch
from
53a54ea
to
8bbc934
Compare
|
@markerikson I don't know why it uses npm instead and causes an error that triggers yarn, strange |
|
@myNameIsDu looks like commit 8bbc934 added a |
myNameIsDu
force-pushed
the
refactor/test-connect
branch
from
d03b64a
to
a250738
Compare
Yes, then the default order for the CSB is to check the lock file of npm => yarn 😆 It's an interesting thing, but I don't know much about it |
|
@markerikson I found some old skip cases, should I delete them? |
myNameIsDu
force-pushed
the
refactor/test-connect
branch
from
fd9bc2b
to
93f0376
Compare
| @@ -2020,6 +2382,7 @@ describe('React', () => { | |||
|
|
|||
| rtl.render( | |||
| <ProviderMock store={store}> | |||
| {/*// @ts-ignore */} | |||
There was a problem hiding this comment.
❓ just out of curiosity, what issues are we ignoring in this area of the code?
There was a problem hiding this comment.
Wow, I was going to talk to you about that, The wrapper returned by connect does not receive the context type
I think this is an advanced use of connectAdvanced, or we need to tweak the original type
There was a problem hiding this comment.
Hmm. Yeah, it's entirely possible that's a bug in the types.
Let's leave the ignore in there for now, and maybe leave a // TODO comment saying to re-investigate it later?
There was a problem hiding this comment.
That's a good idea. I think I can fix it after this PR #1771 is done
test/components/connect.spec.tsx
Outdated
| spy.mockRestore() | ||
| }) | ||
|
|
||
| // it.skip('should throw when trying to access the wrapped instance if withRef is not specified', () => { |
There was a problem hiding this comment.
✋ Yeah, this is dead - getWrappedInstance() went away in version 6, and I guess we never removed this test. Delete it.
There was a problem hiding this comment.
This descript is also all skip, I can also delete it directly?
There was a problem hiding this comment.
yeah, let's delete that one too
|
Awright, let's get this in. Thank you so much for working on this! |
Refactor test connect
link: #1737