webpack-contrib / sass-loader

Compiles Sass to CSS
MIT License
3.91k stars 427 forks source link

Duplicate Imports #145

Closed JoeStanton closed 7 years ago

JoeStanton commented 9 years ago

I know there has been plenty of discussion around this in #31, and it looks like the issue should have been 'resolved', but it's still happening for us.

Importing a stylesheet so we can benefit from mixins, extend styles etc. in our CSS modules causes the imported file to be included twice in the resulting output. This happens even if we use Webpack's DedupePlugin.

components/a/a.scss:

  @import "variables.sass"

components/b/b.scss:

  @import "variables.sass"

Are we doing something wrong? Or is there a better way to solve this problem?

jhnns commented 9 years ago

This is a problem of Sass in general afaik (see here and here). Could you check if the problem is also present when you're using node-sass directly? If that's the case, this is the probably the wrong repo for that issue.

greaber commented 8 years ago

@jhnns, I ran into the same issue. No doubt it is a problem of Sass in general, but I had thought the sass-loader was intercepting @import statements in Sass and could do something clever.

In any case, what is the recommended practice? Should I just be careful that no file is ever @imported twice? This would probably mean removing all @import statements from my per-component Sass files and just @importing every Sass file I use other than the per-component files in one big main.scss file. Is this the right way to do it?

(As an aside, I would also be OK with , and see possible benefits of, ditching Sass in favor of some JavaScript-based stylesheet language like JSS, but I am worried there might be issues that I haven't thought of converting a Sass codebase to JSS, and I am concerned that there are a million different proposals for styling React right now, and JSS doesn't seem to have particularly separated itself from the crowd. Confusing times!)

jhnns commented 8 years ago

There are helper mixins like sass-import-once which prevents double imports. However, this requires you to wrap your whole code with curly braces and to find a unique id for your module (usually the file path within the project). Personally, I find that too annoying.

After fiddling around with double imports I found out that it's not actually a big problem. Gziping your static assets eliminates code duplication very efficiently. You can try that out for your specific case by creating two versions: one with sass-import-once and a gzipped one. Sure, in the final result, the gzipped version contains duplicated style rules. However, I bet the performance penalty would still not be measurable.

but I had thought the sass-loader was intercepting @import statements in Sass and could do something clever.

Yes, that would be possible and I'm actually planing to do something. However, rubysass is going to ship this with 4.0.0 and libsass will probably also follow. So I was hoping this would already be resolved by now :grin:.

If you are open to ditch Sass, I can also recommend LESS. I know, there are endless debates about Sass vs. LESS and in the end they are always pointless. However, there are two things LESS is actually handling better:

And in the end, Sass and LESS are very familiar. Actually, I'm using both in different projects and it's not a big deal.

jsg2021 commented 8 years ago

If you plan on doing StyleSheet-Per-Component (like @JoeStanton s example) Each sass file will compile to a self-contained entity. If you put anything that outputs in your common "variables.scss" it will duplicate. variables, functions, mixins and placeholder selectors don't output. However, placeholder selectors will not merge all extensions across components.

Webpack isn't responsible for this... nor its loaders. Sass (in its current form) cannot handle this. The only way to make this work without duplication is to add a pre-step that synthesises a fake "main.scss" that imports all the components as partials... which also has its issues.

I'm using the sass-per-component model and love it. I use the baggage loader to auto-import sass with my component files. (and the extract text plugin to merge into one css file) I've basically come to accept the limitation, but take comfort in knowing that each component is self-sustaining and can just be stamped... so duplicated styles become less and less of an issue because I should be defining local-to-component styles.

I like @jhnns recommendation. Sass isn't the end-all-be-all tool. Less may be better suited for your task.

greaber commented 8 years ago

Thanks, guys. Maybe I should have known, but I didn't realize I could avoid the duplication by just being careful to avoid importing anything that outputs. And gzip does seem like it can help out if something is still being duplicated. I will keep on with Sass and sass-loader for the time being.

kenotron commented 8 years ago

You can overcome this with https://github.com/at-import/node-sass-import-once - maybe for now, it's a special case until 4.0 comes out!

jhnns commented 8 years ago

Thx for sharing @kenotron :+1:

kenotron commented 8 years ago

I've had great success doing this:

  1. npm install node-sass-importer --save-dev
  2. in webpack config add:
var importer = require("node-sass-importer");

...

sassConfig: {
    importer: importer
}
  1. Make sure your @import's all are relative to the webpack config file (hopefully in root where you run webpack)
wzup commented 8 years ago

@jsg2021 and how do you import @media queries with your approach?

jsg2021 commented 8 years ago

@wzup I don't understand what you are asking? In the approach I mentioned, you would simply define your @media <query> {} blocks in your component's style file... and/or in the containing component's style file.

jogjayr commented 8 years ago

@kenotron I've had trouble with resolving url font files in SCSS when I use node-sass-import-once. Posted a question on SO here: https://stackoverflow.com/questions/36926521/webpack-resolve-url-loader-cannot-resolve-font-url . Any idea what I'm doing wrong?

felixjung commented 8 years ago

I've tried node-sass-importer as suggested by @kenotron. Unfortunately, it seems to have issues with relative imports. We use local styles in our angular project, locating component styles next to the component's javascript. Inside the javascript we import the styles import './foo.scss. This does not seem to work with node-sass-importer, although I thought files would still be resolved using webpack?

Some excerpts from the webpack config

The sass loader should work in the scripts and styles directories:

      {
        test: /\.scss$/, loader: 'style!css?-minimize!sass',
        include: [
          path.resolve(__dirname, 'app/scripts'),
          path.resolve(__dirname, 'app/styles')
        ]
      },

We inject the node-sass-importer. From taking a brief look at the sass-loader code, I think the webpack importer is still pushed on top of this? I.e. the importer reaching node-sass is [node-sass-importer, webpack-importer] (using some pseudo code here...).

  sassConfig: {
    importer: importer
  },

Webpack should look in these places when resolving dependencies:

resolve: {
    root: [
      path.resolve(__dirname, 'app/scripts'),
      path.resolve(__dirname, 'app/icons'),
      path.resolve(__dirname, 'bower_components'),
      path.resolve(__dirname, 'app/styles/sass'),
      path.resolve(__dirname, 'test')
    ],
}

Our components all use some styles from our global brand stylesheets, so we need to import the global stylesheets for each component. This leads to a lot of style duplication and makes working with Safari's dev tools impossible. The browser just isn't able to handle all these duplicate rules.

Any ideas on how to fix this?

felixjung commented 8 years ago

Nevermind, I found the issues were related to what we exactly imported in those shared chunks. Fixed it by optimizing that and ensuring we only import mixins, variables, etc.

Thanks for this great loader!

lauterry commented 8 years ago

Hi @felixjung

How did you exactly optimize that please ?

mattaningram commented 8 years ago

@lauterry To clarify, he is insuring that SCSS files imported across multiple components only contain things like variables and mixins which don't actually turn into CSS output. Thus the final CSS output won't have duplicate styling in it.

If you want to import non-variable or non-mixin styles for more than one component, it's more efficient to import that just once at a higher level component or just in a standard CSS file to avoid duplication.

lauterry commented 8 years ago

Thanks @mattaningram for the clarification !

MatthewKosloski commented 8 years ago

@mattaningram

If you want to import non-variable or non-mixin styles for more than one component, it's more efficient to import that just once at a higher level component or just in a standard CSS file to avoid duplication.

I'm trying to add Normalize.css to my stylesheet, and to avoid duplications. I imported it in the high level component; however, it's at the bottom of the style cascade...

I can't include a link to Normalize in my HTML because it's a node module. Also, I really only want one, compressed CSS file.

Consider the following file structure:

components
    Foo
        foo.scss
    Bar
        bar.scss
app.js
app.scss

App.js is an entry point like so:

import React from 'react';
import { render } from 'react-dom';
import Foo from './components/Foo';
import Bar from './components/Bar';
import s from './app.scss';

const app = (
    <div>
        <Foo />
        <Bar />
    </div>
);

render(app, document.getElementById('app'));

Here is app.scss:

@import '~normalize';

Depiction of the outputted bundle, or the "cascade." (I need Normalize on top):

Foo's rules (foo.scss)
Bar's rules (bar.scss)
Normalize.css (app.scss)
mattaningram commented 8 years ago

@MatthewKosloski This was the exact issue I was having in addition to the duplicates, I couldn't find any way to set the import order of SCSS files. This became such a frustration I eventually stopped importing SCSS files in React components and just did it the "old fashioned" way in a big style.scss file which solved all my duplication and order issues, however loses the benefit of only loading the CSS needed for particular components.

That being said the final CSS file gets cached, so it's not much of an impact to load times after the first time they load a page on your site. It also simplifies having to remember which SCSS files you need for each component.

It would be nice if there was some way of setting a universal order priority for component imports, but that would mean one more thing to keep track of and update as you add new .scss files, so for now I'm happy with the traditional approach.

jhnns commented 8 years ago

@MatthewKosloski this issue is not about the ordering of rules, please don't change the topic. I assume that you are using the extract-text-webpack-plugin. In this case, the order of rules dependents on the order of require() calls inside your bundle. Follow-up discussion at: https://github.com/webpack/extract-text-webpack-plugin/issues/200#issuecomment-251261145

dms1lva commented 7 years ago

For the record, unless I messed something up, sass-import-once does not work with the sass loader. It makes sense because the mixin provided by sass-import-once can't keep track of the imports for the specified module since the sass-loader has isolated contexts.

riccardolardi commented 7 years ago

Any update on this? Can't seem to get rid of those duplicates.

phun-ky commented 7 years ago

While the original issue here is related to sass/less/stylus, a fix/hack/patch for this is to use the optimize-css-assets-webpack-plugin in conjunction with extract-text-webpack-plugin to produce, like this: https://gist.github.com/phun-ky/766e5ec9f75eac61c945273a951f0c0b.

If you want to use this in dev, you will have to use a plugin like write-file-webpack-plugin to force webpack to write the file to disk in dev.

jhnns commented 7 years ago

Related performance discussion: https://github.com/webpack-contrib/sass-loader/issues/296#issuecomment-288464857

powah commented 7 years ago

@phun-ky You saved my day! I was encountering the same issue with my SCSS files included several times (9 in my case!) when components included their own dependencies and was really frustrated while searching for a solution to this. Then I tried optimize-css-assets-webpack-plugin as you suggested in the gist and - IT WORKS!

alexander-akait commented 7 years ago

Apparently there is no for this universal and good solution. If someone has an idea how we can to implementing this, i will be glad to see this here https://github.com/webpack-contrib/sass-loader/issues/296.

rw3iss commented 7 years ago

Any ideas on how to get the node-sass-imported / sassConfig property to be recognized by Webpack 3?

UPDATE:

Webpack 3 entry should go within module.rules[X].loader.use[Y].options { importer: nodeSassImporter} ie:

 module: {
        rules: [
            {
                test: /\.scss$/,
                include: APP_DIR,
                exclude: /node_modules/,
                loader: extractSass.extract({
                    use: [
                        { loader: 'css-loader?sourceMap' }, 
                        { 
                            loader: 'sass-loader?sourceMap',
                            options: {
                                importer: nodeSassImporter
                            }
                        }
                    ],
                    fallback: 'style-loader'
                })
            }
        ]
    },

However, it isn't ignoring duplicates, still including them :(

AndyOGo commented 7 years ago

This should be supported by sass-loader.

Because the node-sass API is offering a custom importer option, which is implemented like node-sass-import-once.

Meanwhile maybe sass-loader-once is usable.

Please reopen this issue.

alexander-akait commented 7 years ago

@rw3iss original node-sass works absolutely identical, to avoid double content use minificator

valoricDe commented 6 years ago

@evilebottnawi which minificator? Does it have advantages over optimize-css-assets-webpack-plugin?

rjgotten commented 6 years ago

Neither node-sass-import-once nor sass-loader-once seem to support package imports.

Imho both disqualify as a proper solution to the import-once problem.

akinhwan commented 5 years ago

are there compile time/performance benefits to importing less files?

rw3iss commented 5 years ago

are there compile time/performance benefits to importing less files?

Of course their are, but the main point is... if you import duplicate files, say some file that has code you want to use in any number of components, and those imported files contain actual css (ie. real css, classes descriptors and their properties, etc), then that code will get rendered into the final bundle, and eventually make its way to the browser as well, and this file will have redundant CSS, and will result in a larger css bundle size, and will also take some bit of extra processing by the browser to be rendered. Obviously we only want to send what we need to to the browser.

So, the main workaround for sass usage is to only put functional shared code in your shared files, and still import them just the same. That means only writing shared code as things like mixins, or functions, within sass. When you import files with mixins or functions, things that don't actually output any css by themselves, then only the sass engine uses them in its memory, to be used in the files that are importing them, and because of this, only those component's actual css makes it into the output bundle, but not the imported files css, because their isn't any actual real css in those files, with this approach.

In short, if you had written actual css in the files you're importing, then that css would be written to the output bundle however many times the file is imported, but if your shared file only contains functional stuff, that code is only used by sass internally, and not rendered redundantly during every import. Hope that makes sense, but let me know if you need an example.

akinhwan commented 5 years ago

@rw3iss thanks ryan this was the example i gave on stackoverflow https://stackoverflow.com/questions/58066851/does-removing-redundant-scss-imports-affect-compile-time

in the example there, if variables.scss has a bunch of variables defined like $color-variable: you're saying since thats not a mixin or 'functional'(?) it would definitely be imported duplicate times wherever i import variables.scss

if so and you answer there i will accept and upvote if you have stackoverflow account, thanks!