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

Add JSX source transform for better warnings #716

Merged
merged 1 commit into from
Sep 23, 2016
Merged

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Sep 23, 2016

Fixes #700.
Read about it here: https://twitter.com/dan_abramov/status/779308833399332864.

Test plan:

A. It shows up with npm start:

screen shot 2016-09-23 at 14 26 54

B. npm run build output doesn’t include __source.

C. It shows up with npm test:

screen shot 2016-09-23 at 14 27 44

@gaearon gaearon added this to the 0.5.0 milestone Sep 23, 2016
@ghost ghost added the CLA Signed label Sep 23, 2016
babelLoader.query = babelLoader.query || {};
babelLoader.query.babelrc = false;
// We embed the preset instead of referring to it because otherwise
// `env` options in the preset do not get taken into account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that so? I've seen some presets that use env, e.g. babel-preset-react-native here. I wonder how that works?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we open an issue in Babel, if this is broken? I think it would be best to keep the react-scripts/ejected configurations as similar as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presets can use env. I think it’s our peculiar way of using them (babelrc: false) that might be breaking this. It would be amazing if you could investigate why this happens but I want to ship this today so going to leave it as is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. It seems like env doesn’t work after ejecting 😢 . Then something must be wrong in how I’m using it, but I can’t figure out what.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #720. I can take a look at it at some point. Another peculiarity that we have is the use of require.resolve() instead of require() as the presets usually do. (Not sure if it's related to the issue though.) I quickly tried changing the preset to use require(), but then some plugins become undefined.

Anyway, happy to get this shipped as is, it's a good improvement to the error messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh :( Then we need to figure out how to fix the ejected setup before merging, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured it out, it’s a Babel bug: babel/babel#4539. For now working around it in the preset. To be honest I don’t remember why we use require.resolve() 😄 . Maybe we don’t even need it.

@ghost ghost added the CLA Signed label Sep 23, 2016
@gaearon gaearon merged commit c4a936e into master Sep 23, 2016
@gaearon gaearon deleted the add-jsx-source branch September 23, 2016 15:53
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants