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

feat: Add method to configure loaders rules #509

Merged
merged 12 commits into from Mar 25, 2019

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Feb 2, 2019

This PR is a proposal to resolve #473 and #504 in a clean way.

Encore.configureLoaderRule() is a low-level function, and has for goal to let the user having full access to Webpack loaders rules (what we push into module.rules) and configure them.

This is the implementation of the idea I had in #504 (comment).

For resolving Vue files linting issue, the next example would be the ideal solution I think:

Encore
  .configureLoaderRule('eslint', loader => {
    loader.test = /\.(jsx?|vue)$/
  });

// actually, the equivalent code is something like this:
const webpackConfig = Encore.getWebpackConfig();
const eslintLoader = webpackConfig.module.rules.find(rule => rule.loader === 'eslint-loader');

eslintLoader.test = /\.(jsx?|vue)$/;

return webpackConfig;

For now, only ESLint loader is supported, but we can easily add other loaders.

Let me know what you think of this solution, thanks!

Copy link
Collaborator

@Lyrkan Lyrkan left a comment

Choose a reason for hiding this comment

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

Hi @Kocal,

I didn't review this sooner because I was a bit skeptical about that new method... but all things considered it may not be a bad idea.

My main concern was that it would be really tied to Encore's internals.
For instance we recently changed the css-loader rule in order to support css modules (and there is a pending PR to do the same for the sass-loader, the less-loader and the stylus-loader).
If your method were already available before that change and someone used it to do something like configureLoaderRule('css', (loader) => { /* manipulate loader.use */ }) it would end up not working anymore (use doesn't exist at the root level anymore).

But since I also think that most people will use it to change the test part of the rule (which shouldn't change much) and that there is a warning about it being a low-level function... I'm not against adding it for the following rules:

  • js (or javascript?)
  • css
  • images
  • fonts
  • sass (and maybe an alias to scss?)
  • less
  • stylus
  • vue
  • eslint
  • ts (or typescript?)
  • handlebars

@weaverryan: What's your opinion on this?

lib/WebpackConfig.js Outdated Show resolved Hide resolved
lib/config-generator.js Outdated Show resolved Hide resolved
test/loaders/eslint.js Outdated Show resolved Hide resolved
test/loaders/eslint.js Outdated Show resolved Hide resolved
@Kocal Kocal force-pushed the feat/configure-loader-rule branch 2 times, most recently from cf582b3 to c502e48 Compare February 8, 2019 14:52
@Kocal
Copy link
Contributor Author

Kocal commented Feb 8, 2019

The implementation (with tests) is done for:

  • javascript and its alias js
  • css
  • images
  • fonts
  • sass and its alias scss
  • less
  • stylus
  • vue
  • eslint
  • typescript and its alias ts
  • handlebars

lib/WebpackConfig.js Outdated Show resolved Hide resolved
lib/WebpackConfig.js Outdated Show resolved Hide resolved
lib/config-generator.js Outdated Show resolved Hide resolved
@Kocal
Copy link
Contributor Author

Kocal commented Feb 25, 2019

By the way, should I start working (now) on a PR for symfony/symfony-docs?

@weaverryan
Copy link
Member

Hey @Kocal!

I'm for this change - it's one of the ugliest things to try to hack into the Encore config. I merged a PR ahead of this - could you fix the conflict?

About the docs, I think the feature is already mostly documented in the code (you've done a nice job). But, I think it would probably be worth mentioning here https://symfony.com/doc/current/frontend/encore/advanced-config.html

Thanks!

@Kocal Kocal force-pushed the feat/configure-loader-rule branch from 2d30831 to 01880cf Compare March 1, 2019 16:27
@Kocal
Copy link
Contributor Author

Kocal commented Mar 1, 2019

Hey,

I totally fucked up my rebase+force push, I didn't checked that my local branch was outdated and commits were reduced from ~10 to 3 😅 Thanks to Travis and GitHub, I was able to retrieve what I've done (even if that's not really funny to manually apply 7 patches).

Some of my tests broke due to last changes on Encore (in particular Sass, SCSS, Stylus... loaders). I will fix them and ping you when I finished.

Also, I will work ASAP on a PR for symfony-docs :)

@Kocal
Copy link
Contributor Author

Kocal commented Mar 1, 2019

PR opened on symfony/symfony-docs#11075

@Kocal
Copy link
Contributor Author

Kocal commented Mar 21, 2019

Friendly bump 👀

@weaverryan
Copy link
Member

Thank you @Kocal!

@weaverryan weaverryan merged commit 94da0c2 into symfony:master Mar 25, 2019
weaverryan added a commit that referenced this pull request Mar 25, 2019
…erryan)

This PR was merged into the master branch.

Discussion
----------

feat: Add method to configure loaders rules

This PR is a proposal to resolve #473 and #504 in a clean way.

`Encore.configureLoaderRule()` is a low-level function, and has for goal to let the user having full access to Webpack loaders rules (what we push into `module.rules`) and configure them.

This is the implementation of the idea I had in #504 (comment).

For resolving Vue files linting issue, the next example would be the ideal solution  I think:
```js
Encore
  .configureLoaderRule('eslint', loader => {
    loader.test = /\.(jsx?|vue)$/
  });

// actually, the equivalent code is something like this:
const webpackConfig = Encore.getWebpackConfig();
const eslintLoader = webpackConfig.module.rules.find(rule => rule.loader === 'eslint-loader');

eslintLoader.test = /\.(jsx?|vue)$/;

return webpackConfig;
```

For now, only ESLint loader is supported, but we can easily add other loaders.

Let me know what you think of this solution, thanks!

Commits
-------

94da0c2 language tweak
9dd6ca4 chore(tests): correct some tests due to last feature for CSSModules
4dbe443 chore(cr): add real aliases support
729a696 feat: add warning
5cf3d02 feat: add missing loaders, add more tests
86dc788 refactor(test): `configureLoaderRule()` will be easier to test
39cb9bd chore(cr): move tests into
bc0e9bf chore(cr): add shortcut function applyRuleConfigurationCallback
bc1d025 chore(cr): more user-friendly error for valid configurable loaders
01880cf Add method on "Encore"
49a4258 add tests
9892516 feat: implement "configureLoaderRule" method
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Mar 25, 2019
…derRule()` method (Kocal, Lyrkan, weaverryan)

This PR was merged into the 3.4 branch.

Discussion
----------

Encore (advanced): add documentation for `configureLoaderRule()` method

Hey,

This PR add a new entry in the advanced configuration of Webpack Encore, which explains how to use the new method `configureLoaderRule()`.

Ref: symfony/webpack-encore#509

Commits
-------

e3d43b9 minor tweak
71fd3b7 Apply suggestions from code review
c02322c Encore (advanced): add documentation for `configureLoaderRule()` method
@Kocal Kocal deleted the feat/configure-loader-rule branch March 26, 2019 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible incompatibility with enableEslintLoader() and eslint-vue-plugin
3 participants