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

Cleanup Babel PR (ReactFreshPlugin) #16340

Merged
merged 3 commits into from
Aug 13, 2019

Conversation

lunaruan
Copy link
Contributor

@lunaruan lunaruan commented Aug 9, 2019

This PR addresses the comments in #16297

In particular, for ReactFreshBabelPlugin, @gaearon mentioned that we could modify the ReactFreshBabelPlugin to not throw an error if it is run in a test (if expect is a function it is most likely in a test environment) instead of setting a precedent for modifying the jest config.

@sizebot
Copy link

sizebot commented Aug 9, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@@ -11,7 +11,7 @@ export default function(babel) {
if (typeof babel.getEnv === 'function') {
// Only available in Babel 7.
const env = babel.getEnv();
if (env !== 'development') {
if (env !== 'development' && typeof expect !== 'function') {
Copy link
Collaborator

@acdlite acdlite Aug 9, 2019

Choose a reason for hiding this comment

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

@gaearon My understanding was that one of the purposes of this error is to prevent people from enabling the Fresh plugin when compiling their unit tests. Doesn't this change mean that consumers of this plugin who use Jest won't get the error below?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We actually do run it in tests in fbsource. So I don't know that it's necessarily a bad idea.

The primary purpose is to prevent people from including it in prod setups.

@gaearon gaearon merged commit 89bbffe into facebook:master Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants