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

Duplicate Imports #145

Closed
JoeStanton opened this issue Aug 13, 2015 · 33 comments
Closed

Duplicate Imports #145

JoeStanton opened this issue Aug 13, 2015 · 33 comments

Comments

@JoeStanton
Copy link

@JoeStanton JoeStanton commented Aug 13, 2015

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
Copy link
Member

@jhnns jhnns commented Aug 14, 2015

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
Copy link

@greaber greaber commented Jan 22, 2016

@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
Copy link
Member

@jhnns jhnns commented Jan 22, 2016

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 😁.

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:

  • It's possible to import other files without including the actual CSS via @import (reference)
  • It's possible to rewrite url() statements thus enabling modules to just use the actual file path of an image or font without using some helper variable like $font-path.

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
Copy link

@jsg2021 jsg2021 commented Jan 22, 2016

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
Copy link

@greaber greaber commented Jan 22, 2016

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
Copy link

@kenotron kenotron commented Feb 3, 2016

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
Copy link
Member

@jhnns jhnns commented Feb 4, 2016

Thx for sharing @kenotron 👍

@kenotron
Copy link

@kenotron kenotron commented Feb 4, 2016

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
Copy link

@wzup wzup commented Mar 16, 2016

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

@jsg2021
Copy link

@jsg2021 jsg2021 commented Mar 16, 2016

@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
Copy link

@jogjayr jogjayr commented Apr 29, 2016

@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
Copy link

@felixjung felixjung commented Jul 6, 2016

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
Copy link

@felixjung felixjung commented Jul 6, 2016

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
Copy link

@lauterry lauterry commented Jul 29, 2016

Hi @felixjung

How did you exactly optimize that please ?

@mattaningram
Copy link

@mattaningram mattaningram commented Aug 24, 2016

@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
Copy link

@lauterry lauterry commented Aug 24, 2016

Thanks @mattaningram for the clarification !

@MatthewKosloski
Copy link

@MatthewKosloski MatthewKosloski commented Oct 3, 2016

@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
Copy link

@mattaningram mattaningram commented Oct 3, 2016

@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
Copy link
Member

@jhnns jhnns commented Oct 4, 2016

@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: webpack-contrib/extract-text-webpack-plugin#200 (comment)

@dms1lva
Copy link

@dms1lva dms1lva commented Nov 29, 2016

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
Copy link

@riccardolardi riccardolardi commented Dec 2, 2016

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

@phun-ky
Copy link

@phun-ky phun-ky commented Jan 16, 2017

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
Copy link
Member

@jhnns jhnns commented Mar 22, 2017

Related performance discussion: #296 (comment)

@powah
Copy link

@powah powah commented Apr 14, 2017

@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
Copy link
Member

@alexander-akait alexander-akait commented May 23, 2017

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 #296.

@rw3iss
Copy link

@rw3iss rw3iss commented Aug 26, 2017

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
Copy link

@AndyOGo AndyOGo commented Sep 18, 2017

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
Copy link
Member

@alexander-akait alexander-akait commented Sep 18, 2017

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

@valoricDe
Copy link

@valoricDe valoricDe commented Oct 19, 2017

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

@rjgotten
Copy link

@rjgotten rjgotten commented Feb 23, 2018

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
Copy link

@akinhwan akinhwan commented Sep 23, 2019

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

@rw3iss
Copy link

@rw3iss rw3iss commented Sep 23, 2019

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
Copy link

@akinhwan akinhwan commented Sep 23, 2019

@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!

dbjorge added a commit to microsoft/accessibility-insights-web that referenced this issue Mar 30, 2020
#### Description of changes

This PR fixes perf issues when trying to use Chromium's dev tools (particularly, the "inspect element" view) with our web extension. The issue was that every individual CSS module that imported `colors.scss` was resulting in its own copy of all the color variables being included in the final bundled css files (see webpack-contrib/sass-loader#145); `detailsview.css` had dozens of copies, each of which was being considered and then rendered as "overriden" in the dev tools UI for every element, which was causing lots of slowdown.

This PR resolves that issue by splitting out the parts of `colors.scss` that define actual CSS `--var` statements from the parts that define the SCSS variables our modules use directly. The SCSS variables remain in place in `colors.scss` (so no change to most of the consumers is required), but the `--var` styles are moved into a separate `/common/styles/root-level-only/color-definitions.scss` file that is intended to be imported only by root-level scss files. I then updated the root-level SCSS files (covering popup/details view/guidance pages/injected/reports/ui package) to directly reference the new root-level-only `.scss`.

I did a similar split for `common.scss`, though most of the actual perf issue came from `colors.scss`.

I verified that there didn't seem to be any styling regressions in:
* Injected page dialog or highlight boxes
* Details view
* Popup
* Guidance pages
* Generated reports (fastpass and assessment)
* UI package
* Unified

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->
- [x] Addresses an existing issue: #1503
- [x] Ran `yarn fastpass`
- [n/a] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See `CONTRIBUTING.md`.
- [n/a] (UI changes only) Added screenshots/GIFs to description above
- [n/a] (UI changes only) Verified usability with NVDA/JAWS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet