warsawjs / topics-manager

App for choosing subjects and signing up as trainers for workshops
MIT License
10 stars 1 forks source link

Add default to ActivityIndicator export #13

Closed greglobinski closed 6 years ago

greglobinski commented 6 years ago

The issue is described here https://github.com/airbnb/enzyme/issues/1546 Because ActivityIndicator was exported as 'named export' and declared as an arrow function the shallow method in TopicsList.test.js when topics are loading test rendered it as <Component /> not as it should as <ActivityIndicator />

There was a different solution, changing ActivityIndicator to a normal named function

export function ActivityIndicator() {
    return (
        <p>Loading</p>
    );
}

but I've chosen to make it a default export. This way the code is more consistent

kgajowy commented 6 years ago

@greglobinski if I understood the point correctly, shouldn't be some snapshots replaced?

greglobinski commented 6 years ago

@kgajowy No, the snapshot is OK,

exports[`<TopicsList> when topics are loading should match snapshot 1`] = `
<Section>
  <Text
    type="primary"
  >
    Zgłoszone tematy
  </Text>
  <ActivityIndicator />
</Section>
`;

but the test 'when topics are loading' fails (on my localhost) because the shallow method returns something like this

<Section>
  <Text
    type="primary"
  >
    Zgłoszone tematy
  </Text>
  <Component />
</Section>
`;

the <Component /> instead <ActivityIndicator />

greglobinski commented 6 years ago

@kgajowy The guy reporting the issue has almost the same stack Enzyme | 3.3.0 and React | 16.2.0 as we

greglobinski commented 6 years ago

@kgajowy If there is a different behavior of jest/enzyme on my localhost and yours it could be because of the issue, because of it I have to use the default jest installed with the create-react-app and that is the 20.0.4, the jest installed in the repo is 22.4.3, maybe that's the reason.

greglobinski commented 6 years ago

@kgajowy With the workaround I've found, the issue has gone, so maybe it's not necessary to merge the PR. However, it makes the code more consistent, after all there is only one export in the file (ActivityIndicator.js), so why not default.

Tuhaj commented 6 years ago

yep, it's more consistent so we merge!