Skip to content

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

Closed
tizmagik wants to merge 8 commits into
react:masterfrom
tizmagik:master
Closed

Warn on JSX file extension with check-filename-webpack-plugin#361
tizmagik wants to merge 8 commits into
react:masterfrom
tizmagik:master

Conversation

@tizmagik

Copy link
Copy Markdown
Contributor

Fixes#290

@ghostghost added the CLA Signed label Aug 4, 2016
@tizmagiktizmagik mentioned this pull request Aug 4, 2016
@gaearon

Copy link
Copy Markdown
Contributor

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 (react#362)
# Conflicts:
#	config/webpack.config.dev.js
@tizmagik

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

propose

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

@ghostghost added the CLA Signed label Aug 12, 2016
@gaearon

Copy link
Copy Markdown
Contributor

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 😄

@gaearongaearon closed this Sep 3, 2016
@locklockBot locked and limited conversation to collaborators Jan 22, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tizmagik@gaearon@andreypopp