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.

Sign up for GitHub

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

Jump to bottom

Including .css files with @import is non-standard behaviour which will be removed in future versions of LibSass. #2362

Closed
ghost opened this issue May 2, 2018 · 69 comments
Closed

Including .css files with @import is non-standard behaviour which will be removed in future versions of LibSass. #2362

ghost opened this issue May 2, 2018 · 69 comments

Comments

@ghost
Copy link

ghost commented May 2, 2018 

Including .css files with @import is non-standard behaviour which will be removed in future versions of LibSass.
Use a custom importer to maintain this behaviour. Check your implementations documentation on how to create a custom importer.

What's the recommend way to import .css files now ? "Use a custom importer to maintain this behaviour." Can't find this in the documentation of SASS or here.

Regards
@xzyfer
Copy link
Contributor

xzyfer commented May 3, 2018

Our recommendation is to not import .css files. Doing so makes you're code non-portable and tightly couples you to a specific implementation.

In node-sass@v5 (as with all other Sass implementations):

@import "foo.css";

Will be transformed to

@import url(foo.css);

This output instructs the browser to download the file foo.css.

If you wish to inline the contents of foo.css into the output CSS file then we recommending using a CSS minification step in you build process.

All popular CSS minification libraries will inline @imports see

@sass sass deleted a comment from badtant May 3, 2018
@webdevan
Copy link

webdevan commented May 5, 2018

I don't really see the value in deprecating this behavior. Is there a technical constraint?

I work on a few projects which import css files directly from npm modules because they aren't written in scss. In my use case, I have a separate scss file for each of my components and some of them extend third party npm modules by importing the css. So I can't easily rename them to scss.

@import url() is a nice option to have, but I think there are still good reasons to be able to import css as if it is scss. The syntax is compatible after all. That's why everyone adopted scss over sass in the first place because it was backwards compatible. And now it's not?

@ezze
Copy link

ezze commented May 5, 2018

I still want third-party CSS files to be a part of my final single CSS built with SASS. When you use module bundlers such as Webpack relying on @import url(third-party.css) is not a good option because you have to care about copying CSS files to destination by yourself... It introduces an additional headache, especially, when your visual components styled with SASS are splitted into separate npm modules.

@xzyfer
Copy link
Contributor

xzyfer commented May 5, 2018 via email

@xzyfer
Copy link
Contributor

xzyfer commented May 5, 2018 via email

@jhildenbiddle
Copy link

Why not ship v5 spec-compliant by default with a flag that provides the current CSS import behavior for those of us who depend on it?

node-sass file.scss --import-css true

The flag could be added to a 4.x release with an updated deprecation warning that suggests setting the new flag to ensure 4.x => 5.x updates go smoothly.

I tried transferring @import inlining duties from node-sass to a separate minification library per @xzyfer 's suggestion. It works as expected for the final CSS, but adding yet-another-processor to the CSS toolchain broke sourcemaps. I am also using postcss, so identifying the issue(s) will take time and may be challenging to resolve. This is my main concern with removing the existing behavior: it breaks things, adds complexity, and the fix won't always be as simple as just adding a new lib/plugin.

All that said, I appreciate the need to align with the Sass specification. I just think a lot of people would prefer to set a flag and keep their working build working instead being forced to make changes that won't necessarily provide any benefit.

@xzyfer
Copy link
Contributor

xzyfer commented May 6, 2018 via email

@xzyfer
Copy link
Contributor

xzyfer commented May 6, 2018 via email

@webdevan
Copy link

webdevan commented May 6, 2018

I'm lacking in the background knowledge to understand why the change, but to me it just sounds like the "Sass spec" is the problem and it should just be adjusted to allow @import of css again. I found this issue: sass/sass#556 (comment) which has been locked since 2015 and on Apr 5 it got tagged as "planned feature".

Not sure what the best approach is to handle this, but I think that any changes to syntax, cli flags or additional modules is going to cause headaches for a lot developers. Ideally, we want Sass spec to change to treat .css as .scss?

@badtant
Copy link

badtant commented May 6, 2018

At first I used https://www.npmjs.com/package/node-sass-css-importer
I use webpack and then realized that even though node-sass won't import my css-files, css-loader will. So if you use that, there is no need to worry.

@zaidan zaidan mentioned this issue May 6, 2018
Merged
iaincollins added a commit to iaincollins/nextjs-starter that referenced this issue May 7, 2018
@iaincollins
Release 6.0.1 - Upgrading all dependancies
ae141b8 
* Upgrades _error.js to use withRouter to access URL properties.
* Brings all dependancies up to date with the latest releases.
* node-sass has been upgraded to 4.8.3 but not 4.9.0 due to this issue that relates to an issue with the sass spec and the deprecation of including CSS files from SASS. sass/node-sass#2362

This is horrible change to the the spec as it makes it harder to integrate with third party projects unless they also use SASS. It’s currently only generating a warning in node-sass 4.9 but am holding back on upgrading at all until we know if/how it will be resolved in node-sass@5.
@TheJaredWilcurt
Copy link

@xzyfer
You tell us to go discuss it in sass/sass, but don't link to an existing thread. There is one, but it's been locked:

The last comment on there from @hcatlin is "We should let people use this feature".

The last comment from @chriseppstein is "I don't care if LibSass allows importing of extensionless CSS files, I only care when it contains the extension".

That thread discusses a solution that has not been implemented yet. Why remove this feature before the official solution is implemented? You have not done a good job of explaining the harm that is done by leaving this feature as is until official Sass-language support is added for a new way of doing this.

Though, like Chris, I personally see nothing wrong with the current implementation of just using an extensionless path. Extensionless paths are not supported in regular CSS, so it does not override any existing CSS syntax. I always assumed that was an intentional design decision to allow importing of CSS files in that manner, ever since I first started using Sass 4 years ago. I thought it was a clever and intuitive solution, which has worked in every Sass processor I've ever used.

Note: The error/warning mentioned in the first post of this issue occurs when using this:

@import "../node_modules/normalize.css/normalize";

Also, I will say that a solution that only supports node_modules is pretty stupid, just continue supporting CSS imports via extensionless paths. It's such a good feature from a usability standpoint.

@joemaller
Copy link

I agree that CSS-as-Sass imports have been terribly confusing. Still, I've been relying upon them for years. (Mostly just pulling in Normalizer from node_modules)

However, all our CSS pipelines also use Autoprefixer via PostCSS, and adding PostCSS-Import for inlining CSS imports is really trivial.

After an initial WTF moment, I actually prefer this. It's obvious that CSS imports are different from Sass imports and inlining via PostCSS makes sense.

There are a couple caveats: First, Sass hoists native CSS @imports to the top of output files, which might differ from where the imports occur -- doesn't change how it all works though. Second, as mentioned above, Sass minification obviously doesn't affect inlined imports, so production pipelines should use a separate minifier, I just added cssnano to the list of PostCSS plugins.

@ryanelian
Copy link

IMHO this feature / bug should not be deprecated before the official Sass module system is developed...

https://github.com/sass/language/blob/master/proposal/module-system.md

These are goals that are based less on philosophy than on practicality. For the most part, they're derived from user feedback that we've collected about @import over the years.

  • Using CSS files. People often have CSS files that they want to bring into their Sass compilation. Historically, @import has been unable to do this due to its overlap with the plain-CSS @import directive and the requirement that SCSS remain a CSS superset. With a new directive name, this becomes possible.

@sass sass deleted a comment from SagarNaliyapara May 9, 2018
@xzyfer
Copy link
Contributor

xzyfer commented May 11, 2018 via email

@dinandmentink
Copy link

Ah. I was mistaken. Thanks for clarification!

@chriseppstein chriseppstein mentioned this issue May 12, 2018
Closed
@xzyfer
Copy link
Contributor

xzyfer commented May 12, 2018 via email

@TheJaredWilcurt
Copy link

It seems as though we may be talking about two different things in this thread, and it is causing confusion.

  1. Using @import "file"; to import a .css file and have it inlined. Doing this is in accordance with the @import documentation, and should continue to be allowed. However the documentation should make this more explicitly clear. This functionality should remain intact as it is part of most developer's workflows, solves a common problem, and adheres to the language specification. We should stop displaying warning/error messages in node-sass when people import extensionless CSS files.
  2. Using @import "file.css"; and expecting it to inline the styles. This functionality violates the Sass language specification. It should be removed. The expected behavior should be that the outputted CSS file contains the same original import code @import "file.css";. This will keep node-sass in line with the Sass language specification, and the goal of being a superset of CSS. Those currently relying on importing CSS in this manner for the purpose of inlining the styles should continue to see the warning/error message in node-sass. However the message should be udpated to be more helpful, by advising developers to remove the .css extension if they would like for the code to be imported in the same manner as .sass and .scss files. I think linking to an online explanation page may be good as well (could be a closed and locked GitHub issue with a link to this one for discussion). Something that gives more information behind why they are seeing the message and what steps to take to resolve it.

Here is a rough example of the explanation text that could be used:

Node-sass has detected you are importing a CSS file in your Sass code. Soon we will change how we handle the importing of CSS files in node-sass. You should update your code to follow one of these two patterns depending on your intention.

  1. If your intention is to have the browser make a network request to retrieve this CSS file at page load, then you should update your code from:
@import "file.css";

to:

@import url('file.css');
  1. If your intention is to have the CSS imported and inlined in the same manner as a .sass or .scss file, then you should remove the .css file extension, like so:
@import "file";

To read more about this change and the discussion around it, see Issue #2362.

Node-sass should be updated so that the warning messages do not occur on extensionless imports, and on imports using url(). The remaining warning should be updated to be more helpful and point to additional information, like the suggestion above.

@xzyfer
Copy link
Contributor

xzyfer commented May 13, 2018 via email

@farneman
Copy link

farneman commented May 13, 2018 •
edited

I believe the relevant lines of the Sass docs are:

@import takes a filename to import. By default, it looks for a Sass file to import directly, but there are a few circumstances under which it will compile to a CSS @import rule:

  • If the file's extension is .css.
  • If the filename begins with http://.
  • If the filename is a url().
  • If the @import has any media queries.

If none of the above conditions are met and the extension is .scss or .sass, then the named Sass or SCSS file will be imported. If there is no extension, Sass will try to find a file with that name and the .scss or .sass extension and import it.

From the way I read that last line it seems to indicate that it doesn't matter wether the @import reference has an extension, only the extension of the actual file that it's pointing to. I could be misinterpreting it though.

@xzyfer
Copy link
Contributor

xzyfer commented May 13, 2018 via email

@jhildenbiddle
Copy link

jhildenbiddle commented May 13, 2018 •
edited

Based on @farneman 's quote from the Sass docs, I'd argue that the spec is ambiguous as to how extension-less CSS files will be handled.

@import takes a filename to import. By default, it looks for a Sass file to import directly, but there are a few circumstances under which it will compile to a CSS @import rule:

  • If the file's extension is .css.
  • If the filename begins with http://.
  • If the filename is a url().
  • If the @import has any media queries.

An extension-less CSS file does not meet this criteria.

If none of the above conditions are met and the extension is .scss or .sass, then the named Sass or SCSS file will be imported.

An extension-less CSS file does not meet this criteria.

If there is no extension, Sass will try to find a file with that name and the .scss or .sass extension and import it.

Note the emphasis (mine). An extension-less CSS file meets none of the above criteria, so it's left up to interpretation as to how they should be handled.

@xzyfer
Copy link
Contributor

xzyfer commented May 13, 2018 via email

@webdevan
Copy link

I think @xzyfer's points are valid.

My opinion:

  1. SASS shouldn't have used @import to mean something different to what CSS @import means. They should have used a different keyword to inline imports vs network request.
  2. Node-Sass shouldn't have allowed @import of CSS files to be inline (as convenient as this "feature" was).
  3. Discussion should be aimed at understanding the current use cases and how to mitigate the impact of the change.

The current use cases are importing inline .css files from places (usually ./node_modules/) where SASS was not used.

Personally, I think the easiest way to mitigate this would be to have one extra npm module that we have to install when we upgrade to the new node-sass, if we want all of our @imports to be inline instead of network requested. Perhaps the deprecation notice could also inform us of this extra npm install that we should do. Then, at least it's a once off fix that we we can easily apply to all projects that ever relied on @import 'css-file';

@xzyfer
Copy link
Contributor

xzyfer commented May 13, 2018 via email

@chriseppstein
Copy link
Contributor

chriseppstein commented May 14, 2018 •
edited

I'd argue that the spec is ambiguous as to how extension-less CSS files will be handled.

@xzyfer is right that we need the implementations to match for default behavior. If they don't, a sass file will stop being portable across implementations (if a sass file developed on node-sass imports a css file, even sans extension, that file won't work in other implementations).

But, it is ok as an optional feature that defaults to false. So node-sass can be perfectly compliant as a Sass implementation if it decides to provide a configuration option to enable css imports as long as it respects the documented rules about what causes an @import to become a pure css-based import. Then sass authors know when they are depending on a feature that is non-standard and requires some additional set-up.

In the long term, libSass should add a parser mode that imports css files without parsing them with any additional Sass syntax or semantics. this will ensure forward compatibility with CSS if it ever changes in a way that collides with a Sass enhancement. Such changes do not arrive unannounced, so there is ample time to build this and roll it out with appropriate deprecation warnings.

SASS shouldn't have used @import to mean something different to what CSS @import means. They should have used a different keyword to inline imports vs network request.

We agree. Sass 4 will deprecate @import in favor of @use and Sass 5 will return @import to CSS with no overloaded meaning.

Node-Sass shouldn't have allowed @import of CSS files to be inline

Technically, it's a libSass thing. But I agree. I gave hampton a piece of my mind about this quite some time ago ;)

Personally, I think the easiest way to mitigate this would be to have one extra npm module that we have to install when we upgrade to the new node-sass

Adding eyeglass to your node-sass project adds a custom importer that by default allows all imports without an extension to resolve to css files. Plain CSS files are parsed as scss syntax.

Eyeglass also makes it easy to import from node_modules if an npm package is configured as an "eyeglass module" or if you manually configure your project to describe the layout of those node packages that are not specifically designed to work with eyeglass.

Eyeglass is a bit heavy-weight for this single use-case, but it exists now and adds a lot features and useful functionality for distributing Sass files as npm modules. I built it as the successor to Compass, with a focus on helping the community collaborate.

@nschonni nschonni mentioned this issue Jun 28, 2018
Closed
This was referenced Jul 2, 2018
Merged
Closed
@xzyfer
Copy link
Contributor

xzyfer commented Jul 2, 2018

The revert has landed in LibSass. We expect a new node-sass release sometime this weekend.

@curran

This comment has been minimized.

Sign in to view
@xzyfer

This comment has been minimized.

Sign in to view
@curran

This comment has been minimized.

Sign in to view
@esc-mhild esc-mhild mentioned this issue Jul 18, 2018
Closed
@mmtr mmtr mentioned this issue Jul 18, 2018
Closed
4 tasks
@ghost ghost mentioned this issue Jul 21, 2018
Closed
tai2 added a commit to tai2/webpacker-react-example that referenced this issue Jul 23, 2018
@tai2
Stop importing css from scss
bca29a7 
see: sass/node-sass#2362
@moonglum

This comment has been minimized.

Sign in to view
xiaohutai added a commit to xiaohutai/bolt-theme-starter that referenced this issue Aug 1, 2018
@xiaohutai
Import normalize.css
6f2029f 
WARNING: This may be deprecated:

Including .css files with @import is non-standard behaviour which
will be removed in future versions of LibSass.

Use a custom importer to maintain this behaviour. Check your
implementations documentation on how to create a custom importer.

See also: sass/node-sass#2362
a-liebhardt pushed a commit to a-liebhardt/luzerner-rollenspieltage that referenced this issue Aug 3, 2018
Cloned faltpickr css to scss because of sass/node-sass#2362
84fa178
@Nox04 Nox04 mentioned this issue Aug 5, 2018
Merged
@TokugawaTakeshi TokugawaTakeshi mentioned this issue Aug 11, 2018
Closed
MDev128 added a commit to MDev128/nextjs-starter that referenced this issue Aug 13, 2018
@MDev128
Release 6.0.1 - Upgrading all dependancies
4b185c2 
* Upgrades _error.js to use withRouter to access URL properties.
* Brings all dependancies up to date with the latest releases.
* node-sass has been upgraded to 4.8.3 but not 4.9.0 due to this issue that relates to an issue with the sass spec and the deprecation of including CSS files from SASS. sass/node-sass#2362

This is horrible change to the the spec as it makes it harder to integrate with third party projects unless they also use SASS. It’s currently only generating a warning in node-sass 4.9 but am holding back on upgrading at all until we know if/how it will be resolved in node-sass@5.
@chrismwendt chrismwendt mentioned this issue Aug 19, 2018
Open
@ralyodio

This comment has been minimized.

Sign in to view
@fritzmg

This comment has been minimized.

Sign in to view
@willdurand willdurand mentioned this issue Aug 28, 2018
Merged
@halfnibble

This comment has been minimized.

Sign in to view
@halfnibble halfnibble mentioned this issue Aug 29, 2018
Closed
@ralyodio

This comment has been minimized.

Sign in to view
@nex3

This comment has been minimized.

Sign in to view
@sass sass locked and limited conversation to collaborators Aug 30, 2018
@xzyfer
Copy link
Contributor

xzyfer commented Nov 15, 2018

This is the latest 4.11.0 beta release. You can try it now out by installing the beta.

npm install node-sass@beta

This will land in stable next week.

@xzyfer
Copy link
Contributor

xzyfer commented Dec 9, 2018

No feedback received during beta. This fix has been promoted to stable in 4.11.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests