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

Fix suspense fallback is invalid #2328

Open
wants to merge 5 commits into
base: master
from

Conversation

@lamhieu-vk
Copy link
Contributor

lamhieu-vk commented Feb 8, 2020

Resolved #2200

I updated enzyme-adapter-react-16 to fix issue suspense fallback is invalid. I handled to always wrap React.Suspense component outside of React.Lazy and React.Fallback components in any case.

I passed all tests in master branch and local test by below code:

test('renders suspenseFallback=true', () => {
  const el = <ComponentWrapper />;
  const wrapper = shallow(el, { suspenseFallback: true });
  expect(wrapper.find(Fallback).length).toEqual(1);
  expect(wrapper.find(LazyComponent).length).toEqual(0);
});

test('renders suspenseFallback=false', () => {
  const el = <ComponentWrapper />;
  const wrapper = shallow(el, { suspenseFallback: false });
  expect(wrapper.find(Fallback).length).toEqual(0);
  expect(wrapper.find(LazyComponent).length).toEqual(1);
});

Code cloned from issue to reproduce, it's failed in master branch and passed in this branch.

Note: Lazy component will always display as <lazy /> in debug and If you want to expect this component, should be use .find(Component) instead of .find('ComponentName').

Thanks for review!

@lamhieu-vk lamhieu-vk requested a review from ljharb Feb 8, 2020
@lamhieu-vk lamhieu-vk mentioned this pull request Feb 8, 2020
2 of 13 tasks complete
const renderElement = (elConfig, ...rest) => {
const renderedEl = renderer.render(elConfig, ...rest);

const typeIsExisted = !!(renderedEl && renderedEl.type);

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 8, 2020

Collaborator

when would this not be true?

This comment has been minimized.

Copy link
@lamhieu-vk

lamhieu-vk Feb 9, 2020

Author Contributor

Yep, sometimes type in Element is undefined with $$typeof is (react.element). I'm not sure I know which cases it can be undefined.

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 9, 2020

Collaborator

We've got a bunch of existing helpers for this kind of thing; is there one of those we could use here?

This comment has been minimized.

Copy link
@lamhieu-vk

lamhieu-vk Feb 10, 2020

Author Contributor

I don't see any helper to check is existed type in element. Do you want to me create a helper to use it?

@@ -1722,7 +1722,7 @@ describe('shallow', () => {
});
it('finds LazyComponent when render component wrapping lazy component', () => {
const LazyComponent = lazy(fakeDynamicImport(DynamicComponent));
const LazyComponent = lazy(() => fakeDynamicImport(DynamicComponent));

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 8, 2020

Collaborator

why are these test changes needed?

This comment has been minimized.

Copy link
@lamhieu-vk

lamhieu-vk Feb 9, 2020

Author Contributor

I changes to like user cases example in react docs.

const OtherComponent = React.lazy(() => import('./OtherComponent'));

React.lazy takes a function that must call a dynamic import(). This must return a Promise which resolves to a module with a default export containing a React component.

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 9, 2020

Collaborator

I'm concerned about changing existing tests; instead, let's leave these unchanged, and also add test cases with the thunk?

This comment has been minimized.

Copy link
@lamhieu-vk

lamhieu-vk Feb 10, 2020

Author Contributor

I viewed lazy test of react here. The old test is not correct with user case of react lazy but I don't know it still passed. However, I think we should update it!

@ljharb ljharb added this to v16.6+: memo, lazy, suspense in React 16 Feb 8, 2020
@lamhieu-vk

This comment has been minimized.

Copy link
Contributor Author

lamhieu-vk commented Feb 12, 2020

hi @ljharb , could you please give me a feedback?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
React 16
  
v16.6+: memo, lazy, suspense
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.