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

fix(webpack) correct MiniCssExtractPlugin fix: #18 #19

Merged
merged 1 commit into from
Sep 18, 2020
Merged

fix(webpack) correct MiniCssExtractPlugin fix: #18 #19

merged 1 commit into from
Sep 18, 2020

Conversation

avrahamcool
Copy link
Sponsor Contributor

No description provided.

@3cp
Copy link
Member

3cp commented Sep 16, 2020

@Sayan751 can you help to review?

@Sayan751
Copy link
Member

@avrahamcool Thanks for the PR. Can you please add some details, as why the previous configuration was problematic?

@avrahamcool
Copy link
Sponsor Contributor Author

avrahamcool commented Sep 17, 2020

@Sayan751 Already did in issue #18

@Sayan751
Copy link
Member

@avrahamcool @3cp Although I fundamentally don't see any problem with the current PR, I could not unfortunately reproduce the issue. It would be nice to see the reproduction of the original issue.

@avrahamcool
Copy link
Sponsor Contributor Author

I'll create a small repo for that.

@Sayan751
Copy link
Member

Thanks @avrahamcool , that would be really helpful!

@avrahamcool
Copy link
Sponsor Contributor Author

avrahamcool commented Sep 17, 2020

Thanks @avrahamcool , that would be really helpful!

@Sayan751 https://github.com/avrahamcool/webpack-bundling-problem

@Sayan751
Copy link
Member

@avrahamcool Thanks for the repo. This has helped me understanding the issue. Also found this, which explains the root cause of the issue.

@3cp The proposed change looks good to me.

For completeness, let me say that there is another alternative. I was also playing with the webpack options. IMO it is not that bad to classify the assets into different directories like css, images etc. as compared to a heap of garbage in the dist dir. From that perspective, I tried something like the following (abbreviated).

{
  module: {
    rules: [
      {
        test: /\.css$/i,
        issuer: [{ not: [{ test: /\.html$/i }] }],
        use: extractCss ? [{
          loader: MiniCssExtractPlugin.loader,
          options: {
            publicPath: '../'                                 // <-- L1.1
          }
        }, ...cssRules
        ] : ['style-loader', ...cssRules]
      },
      {
        test: /\.(png|gif|jpg|cur)$/i,
        loader: 'url-loader',
        options: {
          limit: 8192,
          name: 'images/[hash].[ext]'                         // <-- L2
        }
      },
    ]
  },
  plugins: [
    new MiniCssExtractPlugin({
      filename: production ? 'css/[name].[contenthash].bundle.css' : 'css/[name].[hash].bundle.css',      //<--  L1.2
      chunkFilename: production ? 'css/[name].[contenthash].chunk.css' : 'css/[name].[hash].chunk.css'    //<--  L1.3
    })
  ]
}

Note that in L1.2, and L1.3 we are pushing the extracted css files to the css directory. Now if we provide the relative path to the "root" as done in L1.1 using the publicPath the MiniCssExtractPlugin will prefix the urls with that. This allow us to put the images in a different directory named images as done in L2. The generated dist structure will be bit more cleaner I believe with this, as well as correcting the generated URLs.

@3cp 3cp merged commit 39dc435 into aurelia:master Sep 18, 2020
@3cp
Copy link
Member

3cp commented Sep 18, 2020

Thank you both!

@avrahamcool avrahamcool deleted the patch-1 branch September 20, 2020 19:02
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.

3 participants