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.

Sign up for GitHub

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

Jump to bottom

Adds JSX extension support #563

Merged
merged 4 commits into from
Sep 3, 2016
Merged

Adds JSX extension support #563

merged 4 commits into from
Sep 3, 2016

Conversation

tizmagik
Copy link
Contributor

@tizmagik tizmagik commented Sep 3, 2016

Adds JSX file support.

Connects to #290

@tizmagik tizmagik mentioned this pull request Sep 3, 2016
Closed
@ghost ghost added the CLA Signed label Sep 3, 2016
gaearon
gaearon reviewed Sep 3, 2016
View reviewed changes
config/webpack.config.dev.js
@@ -74,7 +74,7 @@ module.exports = {
// https://github.com/facebookincubator/create-react-app/issues/253
fallback: paths.nodePaths,
// These are the reasonable defaults supported by the Node ecosystem.
extensions: ['.js', '.json', ''],
extensions: ['.jsx', '.js', '.json', ''],
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it the last extension before "", not the first.

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, how about after .js?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer Node resolution mechanism to be respected before falling back to JSX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok makes sense.

@ghost ghost added the CLA Signed label Sep 3, 2016
@tizmagik
Copy link
Contributor Author

tizmagik commented Sep 3, 2016

@gaearon ok, updated. FWIW, I think adding JSX support is the right direction 👍

@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2016

👍

Can you please include a test plan?

@ghost ghost added the CLA Signed label Sep 3, 2016
@tizmagik
Copy link
Contributor Author

tizmagik commented Sep 3, 2016 •
edited
Loading

@gaearon for testing, I created a Foo.jsx file and included it in the App.test.js test to make sure it ran fine. The tests pass, but I didn't think it made much sense to include it in the template as part of the app. I also tested via creating JSX components locally via create-react-app my-app.

If you think a JSX example in templates would be helpful, I can create something like Header.jsx and have that be included in App..js, although I think it would be awkward/confusing to have a JS component include a JSX component. In userland apps, I would expect all components to either be JS or JSX.

@ghost ghost added the CLA Signed label Sep 3, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2016

It is slightly confusing that App.jsx works but App.test.jsx doesn’t get picked up automatically.
Adding this to Jest config helps:

testRegex: '(/__tests__/.*|\\.(test|spec))\\.(js|jsx)$',

Perhaps worth making a default in Jest if we proceed with this.

@gaearon gaearon added this to the 1.0.0 milestone Sep 3, 2016
gaearon
gaearon reviewed Sep 3, 2016
View reviewed changes
config/webpack.config.dev.js
@@ -74,7 +74,8 @@ module.exports = {
// https://github.com/facebookincubator/create-react-app/issues/253
fallback: paths.nodePaths,
// These are the reasonable defaults supported by the Node ecosystem.
extensions: ['.js', '.json', ''],
// We also include JSX as a common component filename extension.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would specifically add that we don’t recommend using it but some tools rely on it, and link to the issue.

@ghost ghost added the CLA Signed label Sep 3, 2016
@tizmagik
Copy link
Contributor Author

tizmagik commented Sep 3, 2016

Good catch on the App.test.jsx bit. Although I personally would reserve .jsx strictly for React components, but I could see how it would be confusing if it were not picked up by Jest.

@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2016

Since we’re doing it for the sake of more stubborn tooling, seems like it’s best to be fully consistent.

@gaearon gaearon merged commit 49e4f64 into facebook:master Sep 3, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2016

Thanks!

@gaearon gaearon modified the milestones: 0.4.1, 1.0.0 Sep 3, 2016
stayradiated pushed a commit to stayradiated/create-react-app that referenced this pull request Sep 7, 2016
@tizmagik
Adds JSX extension support (facebook#563)
6512ccd 
* Adds JSX extension support

* PR changes

* Add testRegex

* Add note about not recommending JSX, link to issue
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
@tizmagik @feiqitian
Adds JSX extension support (facebook#563)
50a0a5a 
* Adds JSX extension support

* PR changes

* Add testRegex

* Add note about not recommending JSX, link to issue
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
0.4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@tizmagik @gaearon