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

Warn on JSX file extension with check-filename-webpack-plugin #361

Closed
wants to merge 8 commits into from

Conversation

tizmagik
Copy link
Contributor

@tizmagik tizmagik commented Aug 4, 2016

Fixes #290

@ghost ghost added the CLA Signed label Aug 4, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 4, 2016

Sorry for the churn, can you rebase please?

…react-app

* 'master' of https://github.com/facebookincubator/create-react-app:
  0.3.0-alpha
  Update some deps
  Document configuration and build process (facebook#362)

# Conflicts:
#	config/webpack.config.dev.js
@tizmagik
Copy link
Contributor Author

tizmagik commented Aug 5, 2016

No worries, it's all synced up now, should be good. Let me know if any of the copy in the comments or the error message needs any massaging.

Thanks!

// Warn on using .jsx files, prefer using .js only.
// See https://github.com/facebookincubator/create-react-app/issues/290
new CheckFilenamePlugin({
regex: /\.jsx$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a plugin that can be also solved by a loader (which reuses webpack configuration API):

{
  test: /\.jsx$/,
  loader: 'abort',
  query: {
    message: '[resource]: .jsx extensions are not allowed, use .js extension.'
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an actual loader or do you propose to create one?

Copy link
Contributor

Choose a reason for hiding this comment

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

propose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, too. @gaearon please advise.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reasoning behind choosing loader vs. plugin:

  1. Loader can reuse path matching logic.
  2. Loader functional scope is more restrictive: you see exactly what it affects vs. plugin which can do literally anything under the hood. I'd expect the less custom plugins the better, especially when you eject and start modifying config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I think a loader is the more technically correct solution.

@ghost ghost added the CLA Signed label Aug 12, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2016

Closing now because we support JSX extension since 0.4.1 even though we don’t recommend it.
Read the release notes.

Thanks for sticking with us 😄

@gaearon gaearon closed this Sep 3, 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.

3 participants