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

[BUG] with webpack, inlineView deps (<require from="./style.css"></require>) doesn't work #911

Open
3cp opened this issue Aug 15, 2018 · 11 comments

Comments

@3cp
Copy link
Member

3cp commented Aug 15, 2018

I'm submitting a bug report

  • Library Version:
    0.34.0

Please tell us about your environment:

  • Operating System:
    all

  • Node Version:
    all

  • NPM Version:
    all

  • Browser:
    all

  • Language:
    all

  • Loader/bundler:
    Webpack

Current behavior:

aurelia-webpack-plugin did its job to bring in the inlineView required "./style.css". But our cli webpack.config.js prevented it from working properly.

It's caused by:

// CSS required in JS/TS files should use the style-loader that auto-injects it into the website
// only when the issuer is a .js/.ts file, so the loaders are not applied inside html templates
{
  test: /\.css$/i,
  issuer: [{ not: [{ test: /\.html$/i }] }],
  use: extractCss ? [{
      loader: MiniCssExtractPlugin.loader
    },
    'css-loader'
  ] : ['style-loader', ...cssRules]
},
{
  test: /\.css$/i,
  issuer: [{ test: /\.html$/i }],
  // CSS required in templates cannot be extracted safely
  // because Aurelia would try to require it again in runtime
  use: cssRules
},

The "./style.css" dep loaded from inlineView is NOT from issuer html, so it ended up with style-loader instead of packed as a module.

  • What is the expected behavior?

The dep loaded by inlineView should be treated same as dep loaded by html file.

  • What is the motivation / use case for changing the behavior?

Need to update the rules to avoid this regression. But I don't know webpack enough to fix this.

cc @jods4 this bug prevents aurelia/webpack-plugin#107 from working.

@jods4
Copy link

jods4 commented Aug 15, 2018

@huochunpeng's analysis is correct.
This is a problem with the way loaders are configured.

The issuer tests in config ensure .css dependencies from a .ts file are loaded with style-loader.

This is adequate for import "main.css" pattern. Not standard JS but quite common.

It is not adequate for @inlineView("<require from='main.css'>") because this one would be loaded by aurelia-loader.

It is hard to come up with a config that caters for all needs. If you use import "main.css" you typically don't use <require from='main.css'> and vice-versa.

One possible solution is to never include style-loader by default and add it explicitely in import.
import from "style!main.css".

@3cp
Copy link
Member Author

3cp commented Aug 15, 2018

The solution you proposed is a breaking change for users' source code.
We can do the other way around to avoid breaking users' code.

Since all dynamic loading of css files are traced and ONLY traced by aurelia-webpack-plugin,

  1. from a html template
  2. from an inlineView template
  3. from PLATFORM.moduleName, like @noView([PLATFORM.moduleName('./style.css')]);

When aurelia-webpack-plugin do addDependency('./style.css'), it can add prefix like "css!./style.css". Then they will all be loaded by css-loader. (Tell me if webpack.config.js needs a update).

The non-standard import "./style.css"; is traced by standard webpack tracer, they will end up as module "./style.css" without prefix, which you can enforce style-loader on them.

@jods4 is this feasible in webpack?

@jods4
Copy link

jods4 commented Aug 15, 2018

@huochunpeng
That's quite hard to do. Not everyone uses css!. I use css!less!, others use css!sass! and yet other users can pipe more loaders to do additional transforms.

Webpack is very flexible and the philosophy of webpack-plugin is to embrace this flexibility and let users configure their build, with some reasonable defaults when it comes to Aurelia.

I am not familiar with our CLI but it seems to me that this issue lies into the webpack configuration not the plugin.

I suppose that would be a change in our templates for new projects, wouldn't it? If it's a change in new templates then it can't be a breaking change?

@3cp
Copy link
Member Author

3cp commented Aug 15, 2018

Yes, it's a cli issue. I also sensed adding prefix by webpack-plugin is a violation of webpack design.

But is there any way for webpack-plugin to give a hint (other than just issuer) for later webpack config file that a css file is for dynamic loading?

Maybe, something like: this dep is traced by plugin "AureliaPlugin", not standard tracer.

@3cp
Copy link
Member Author

3cp commented Aug 16, 2018

An update to cli template is unavoidable since the current one has bug in supporting basic aurelia convention. In that sense, it's not a breaking change but a hot fix.

@stevies
Copy link

stevies commented Sep 15, 2018

Any update on this? Is there a work-around to get .scss files working with '@inlineView'?
I think I managed to get it working by changing:

     {
        test: /\.scss$/,
        use: ['style-loader', 'css-loader', 'sass-loader'],
        issuer: /\.[tj]s$/i
      },
      {
        test: /\.scss$/,
        use: ['css-loader', 'sass-loader'],
        issuer: /\.html?$/i
      },

to this:

      {
        test: /\.scss$/,
        use: ['css-loader', 'sass-loader'],
      },
      // {
      //   test: /\.scss$/,
      //   use: ['css-loader', 'sass-loader'],
      //   issuer: /\.html?$/i
      // },

but not sure what other things that might break later. I only have .scss files of my own, but may end up with other 3rd party or external css at some point.
It probably causes me no real pain to not use '@inlineView' - at least for now.

@jods4
Copy link

jods4 commented Sep 15, 2018

@stevies I am not sure there has to be a "work-around" here.

You just need a webpack config that matches your usage. If you don't import "x.scss" in your JS files, then it makes perfect sense to use the simplified rule you're using. That's what I do in my own project.

When it comes to 3rd party, you can always filter node_modules out from your loader config.

@stevies
Copy link

stevies commented Sep 15, 2018

Thanks, I'll keep it simple for now.

@Alexander-Taran
Copy link
Contributor

Can be closed?

@stevies
Copy link

stevies commented Oct 14, 2018

No objections from me.

@3cp
Copy link
Member Author

3cp commented Oct 14, 2018

Still a cli wepack config issue with no solution.

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

No branches or pull requests

4 participants