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(gatsby): update check for default exports #16979

Merged
merged 1 commit into from
Aug 23, 2019

Conversation

sidharthachatterjee
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee commented Aug 23, 2019

    return _react["default"].createElement(_react["default"].Fragment, null);
  };

  return AppShell;
}(_react["default"].Component);

var _default = AppShell;
exports["default"] = _default;

The code above throws with

error #11328 

A page component must export a React component for it to be valid. Please make sure this file exports a React component:

/root/project/e2e-tests/production-runtime/node_modules/gatsby-plugin-offline/app-shell.js

This happens because our check doesn't handle exports["default"]

(babel prior to #16929 output exports.default = _default; but now outputs exports["default"] = _default;)

This PR adds that

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch @sidharthachatterjee any chance we can write an unit test or e2e test to catch this issue? Let's do it in a follow up. I'm happy to do so as well.

@wardpeet wardpeet merged commit d16474d into master Aug 23, 2019
@wardpeet wardpeet deleted the update-check-for-default-exports branch August 23, 2019 08:13
@sidharthachatterjee
Copy link
Contributor Author

sidharthachatterjee commented Aug 23, 2019

@wardpeet Will be painful to write as an end to end because we'll need different versions of babel for the different outputs 😓

But we can add fixtures for each compiled case and write some unit tests I think. What do you think?

@wardpeet
Copy link
Contributor

I didn't really look at the whole file so it was more a go to responds :p So not worth at this point to actually add a test. I also think we have covered all possible options so 👍

@sidharthachatterjee
Copy link
Contributor Author

@wardpeet Yeah, I agree. Let's table this for now. Perhaps in the future, we can improve the heuristic that it uses to check whether there is a default export at all. For now, we're good!

Thanks! 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants