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 new rule no-webpack-loader-syntax #586

Merged
merged 3 commits into from
Sep 26, 2016

Conversation

fson
Copy link
Contributor

@fson fson commented Sep 25, 2016

Webpack allows specifying loaders and their configuration inline in imports using a non-standard syntax in the path. This rule allows disabling imports like this.

It can be useful for people using Webpack who don't want their code to rely on this Webpack-specific feature and we can possibly use it in Create React App to fix facebook/create-react-app#733.

Webpack allows specifying loaders and their configuration inline in imports
using a non-standard import syntax. This rule allows disabling this non-standard
syntax in imports.
@coveralls
Copy link

coveralls commented Sep 25, 2016

Coverage Status

Coverage increased (+0.02%) to 97.83% when pulling 755eb86 on fson:no-webpack-loader-syntax into e3c41ca on benmosher:master.

@coveralls
Copy link

coveralls commented Sep 25, 2016

Coverage Status

Coverage increased (+0.02%) to 97.83% when pulling 755eb86 on fson:no-webpack-loader-syntax into e3c41ca on benmosher:master.

Copy link
Collaborator

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

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

I usually prefer discussing a proposal first, to avoid turning down the down you've done all the work. In this case, I agree with the rule and the implementation looks good too.

I just have some suggestions concerning the formulation, and you're missing a line in the README.md.

Thanks a lot and good work! (but next time, opening an issue would probably be better ;) )

var moduleWithOneLoader = require("my-loader!./my-awesome-module");
```

This syntax is non-standard, so it couples the code using to Webpack. The recommended way to specify Webpack loader configuration is in a [Webpack configuration file](http://webpack.github.io/docs/loaders.html#loaders-by-config).
Copy link
Collaborator

Choose a reason for hiding this comment

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

it couples the code to Webpack


Forbid Webpack loader syntax in imports.

[Webpack](http://webpack.github.io) allows specifying [loaders](http://webpack.github.io/docs/loaders.html) and their configuration inline in imports using a special syntax like this:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion:
Webpack allows specifying the loaders to use in the imports source string using ...

@fson
Copy link
Contributor Author

fson commented Sep 25, 2016

Thanks for the review, I made the changes you suggested.

I also understand why it's a good idea to open an issue for new features like this first, and I'll do it the next time I want something added :)

@coveralls
Copy link

coveralls commented Sep 25, 2016

Coverage Status

Coverage increased (+0.02%) to 97.83% when pulling 43bc4de on fson:no-webpack-loader-syntax into e3c41ca on benmosher:master.

@jfmengels
Copy link
Collaborator

LGTM, thanks :)

@benmosher
Copy link
Member

LGTM too, thanks @fson for the submission (I think I'll use this, too!) and as always, thanks @jfmengels for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Forbid using ! in import paths
4 participants