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

Overriding file loader rules to add exceptions #1423

Closed
tegola opened this Issue Jan 19, 2018 · 26 comments

Comments

Projects
None yet
5 participants
@tegola

tegola commented Jan 19, 2018

I'm using Quill in a Vue component. To avoid loading everything from the distributable build, I have prepared a quill entry file (like this one).

When doing this, however, webpack moves the svg icons used by Quill theme in /fonts/vendor/quill and returns their path, instead of returning the svg content, as Quill expects. I end up with this:

schermata 2018-01-19 alle 11 19 18

This doesn't happen when using the distributable build in the npm package.

Quill's build guide and related webpack example say that icons should be loaded with html loader:
https://github.com/quilljs/webpack-example/blob/master/webpack.conf.js#L39

Now to my question: how can I replace/override Laravel Mix's svg loader so it doesn't match Quill icon paths, and instead setup html-loader just for that path?

Thanks

Edit: A past issue (closed unanswered) asks the same thing: #942.

@ankurk91

This comment has been minimized.

Contributor

ankurk91 commented Jan 20, 2018

Here is how you can add your own loaders.

mix.webpackConfig({
module: {
    rules: [
     // your rules may go here
    // next rules are just example, you can modify them according to your needs
      {
        test: /\.(woff|woff2|eot|ttf|svg)(\?.*$|$)/,
        loader: 'file-loader?name=[name].[ext]?[hash]'
      }
    ]
 }
});

Not tested.
Find mix loaders here

@tegola

This comment has been minimized.

tegola commented Jan 21, 2018

That isn't going to work. I want to exclude the .svg files from being loaded by the default fonts rule in Laravel Mix – which is the one that handles SVGs – and add mine that uses a different loader.

If I just add my own rule for handling only SVGs (or SVGs within Quill's specific path), the default one executes first, and returns back the file path instead of its content.

@JeffreyWay

This comment has been minimized.

Owner

JeffreyWay commented Jan 23, 2018

This should be fixed in the next release, which will be out today.

@JeffreyWay JeffreyWay closed this Jan 23, 2018

@tegola

This comment has been minimized.

tegola commented Jan 24, 2018

Release 2.0 doesn't fix my issue, it just changes the output of quill SVGs in the images dir instead of the fonts dir.

@tegola

This comment has been minimized.

tegola commented Jan 26, 2018

Since this issue is now closed I don't expect any more replies. In the meantime, my custom solution was to add an exception to the webpack rule by cycling through Mix's webpack configuration, like this:

const mix = require('laravel-mix');
const quillIconsRegex = /node_modules\/quill\/assets\/icons\/(.*)\.svg$/;

// Exclude quill icons
Mix.listen('configReady', function(config) {
  const rules = config.module.rules;
  const targetRe = /(\.(png|jpe?g|gif)$|^((?!font).)*\.svg$)/;

  for (let rule of rules) {
    if (rule.test.toString() == targetRe.toString()) {
      rule.exclude = quillIconsRegex;
      break;
    }
  }
});

// Use a custom loader for Quill inline icons
mix.webpackConfig({
  module: {
    rules: [{
      test: quillIconsRegex,
      use: [{
        loader: 'html-loader',
        options: {
          minimize: true
        }
      }]
    }]
  }
});

I'd like a better way to do this though, the file loader regex that I'm using to find the rule could change anytime.

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Jan 26, 2018

I think that would at least require named rules.
Would be nice to be able to customize any rule by referencing and overriding it.
Is that what you would like to see?

mix.webpackConfig({
  module: {
    rules: [{
       name: 'images',
       exclude:  /node_modules\/quill\/assets\/icons\/(.*)\.svg$/
    }]
  }
}
@tegola

This comment has been minimized.

tegola commented Jan 26, 2018

@tpraxl Yes! That would be a nice step forward! Is name in the webpack Rule schema?

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Jan 26, 2018

@tegola

I'm not sure.

If the name property causes problems, then the names could be removed before actually using the rules with webpack.

This is why we would have to look deeper into it.

Also, this maybe is a performance issue, because we're not simply pushing but we would need to merge.

I'm currently too busy, but I'm interested in this as well. If I won't have posted anything new about this topic here in 2 weeks, please remind me :)

@tegola

This comment has been minimized.

tegola commented Jan 26, 2018

Just checked: name is not in the schema and throws an error:

Invalid configuration object. Webpack has been initialised using a configuration object that does not match the API schema.

I'm currently too busy, but I'm interested in this as well. If I won't have posted anything new about this topic here in 2 weeks, please remind me :)

Sure! Thanks.

@ZapevalovAnton

This comment has been minimized.

ZapevalovAnton commented Feb 6, 2018

@tpraxl, I remind you)))

@ZapevalovAnton

This comment has been minimized.

ZapevalovAnton commented Feb 6, 2018

I use a ckeditor and it requires an raw-loader for load svg icons. But I can not override the loader.

@ZapevalovAnton

This comment has been minimized.

ZapevalovAnton commented Feb 6, 2018

@tegola thanks for example, but listen is not a function(((

@tegola

This comment has been minimized.

tegola commented Feb 6, 2018

@ZapevalovAnton You need to use the Mix object (uppercase 'M'), so it's Mix.listen(), not mix.listen(). Mix is always available, it's instantiated by the default Laravel mix config.

@ZapevalovAnton

This comment has been minimized.

ZapevalovAnton commented Feb 6, 2018

@tegola Thanks!)

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Feb 6, 2018

@ZapevalovAnton hehe:) I remember it very well. Yet, I still have time until the fortnight is over. Hopefully, I can look into it then, but it might as well take longer.

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Feb 9, 2018

@tegola could you point me to a resource for the valid webpack schema? I'd like to research valid options first. If there's no such option, we could try to name the rules anyway and remove the names before we feed the rules to webpack. But I'd rather try to avoid complicated hacks if possible.

@tegola

This comment has been minimized.

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Feb 9, 2018

Actually, it looks as if it would be pretty easy to use an object with rule names instead of the rules, merge that with the user config and return an array of rules afterwards. Minimal changes required, all tests are green, but I want to test that manually as well, just to be sure. Just had about half an hour to tinker with the idea.

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Feb 9, 2018

@tegola I'll have a look at it too, as soon as I've got some time.

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Feb 10, 2018

Nope. This attempt introduces all sorts of problems. You could override a given rule, but:

  • This breaks the API (expected object in webpackConfig)
  • You could override a rules test, for example, but what if you wanted to exclude a loader (I'm currently attempting to merge)

I think we need another API method that allows us to explicitly override a rule. I'll think about it.

@tegola

This comment has been minimized.

tegola commented Feb 10, 2018

@tpraxl thanks for spending your time looking into this. I currently have no spare time to dedicate to the project, and don't even know well webpack and laravel mix config builder, actually. I'll keep a look at this thread and help in any possible way.

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Feb 10, 2018

There's an early working version that allows to override, skip and customize (merge) the webpack rules:

https://github.com/tpraxl/laravel-mix/tree/feature/override-and-merge-rules

Still need to test that manually and to update the README before I can post a Pull-Request.

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Feb 10, 2018

I tried to setup a quill example project, can't tell if it looks like it is supposed to and I don't want to spend too much time with quill.

@tegola
You could help me out there in two alternative ways (might increase the chance of getting this feature PR ready and merged into laravel-mix)

You are not wasting your time. I promise that the customization will take effect. The question is: does it have the desired effect?

A) Test this version of laravel-mix with your project setup

Just clone the sources, copy them into your node_modules folder to replace the official laravel-mix version. Then replace your webpack.mix.js workaround with this:

const quillIconsRegex = /node_modules\/quill\/assets\/icons\/(.*)\.svg$/;
Mix.customizeRule.images = (rule, Config) => {
  console.warn('using experimental mix version');
  rule.exclude = quillIconsRegex;
  return rule;
};
mix.webpackConfig({
  module: {
    rules: [{
      test: quillIconsRegex,
      use: [{
        loader: 'html-loader',
        options: {
          minimize: true
        }
      }]
    }]
  }
});

Does it show the warning (experimental) when you run it? Does it work?

B) Give me a working example for the failing quill setup

A complete sample project with quill that shows the problems, so that I can reproduce and tell if my changes work for your use-case.

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Feb 10, 2018

Ok. Got it working with the instructions I gave above

Before:

before

After:

after

Will post the PR soon.

@tegola

This comment has been minimized.

tegola commented Feb 14, 2018

@tpraxl PR looks very nice, I'm gonna give it a spin tomorrow. Great work, thank you!

@tpraxl

This comment has been minimized.

Contributor

tpraxl commented Feb 14, 2018

@tegola Just to avoid irritations: the PR is not merged yet.

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