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

Add option to control CSS inline import behaviour #1656

Closed
xzyfer opened this issue Oct 28, 2015 · 11 comments
Closed

Add option to control CSS inline import behaviour #1656

xzyfer opened this issue Oct 28, 2015 · 11 comments

Comments

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2015

We've added the ability to inline @import CSS partials. This is significantly different from the Ruby Sass behaviour (which does not allow this at all) and is understandable causing pain.

We need a context option to enable/disable this feature. It should be disabled by default.

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

I think the wording is a bit misleading. We only allow imports of css partials (stuff ending with .css will left as an actual import in the code). Also Ruby Sass does not allow this by default, but a few tools (AFAIK compass too) enable this by default for Ruby Sass. I'm also not sure if we really want to disable it by default, since this would be another potentially breaking change.

class CSSImporter < Sass::Importers::Filesystem
  def extensions
    super.merge('css' => :scss)
  end
end
Sass::Plugin.options[:filesystem_importer] = CSSImporter

@xzyfer
Copy link
Contributor Author

xzyfer commented Oct 28, 2015

Full disclosure I still think this feature should be removed. Our directive is to do what Ruby Sass does. Anything deviation from their implementation breaks portability, and is indistinguishable from a bug.

Also Ruby Sass does not allow this by default, but a few tools (AFAIK compass too) enable this by default for Ruby Sass

This isn't our job. This belongs in the realm of tools like wellington and eyeglass.

I'm also not sure if we really want to disable it by default

I vehemently believe it should.

this would be another potentially breaking change

The current behaviour is broken as far as Sass users are concerned.

@mgreter
Copy link
Contributor

mgreter commented Nov 7, 2015

I guess it's clear that I'm in favor of having this feature and just wanted to point out that the breakage is mainly only because I've implemented the error message for ambigous imports for libsass 3.3.0. Beside that we only differ by default in partial resolving (also includeing css extension), and I would argue that normally you don't generate or have a css with the same name beside a partial import. Unsure when I will come around to implement that as an option on the C-API. Contributers welcome ;)

@esr360
Copy link

esr360 commented Nov 9, 2015

I think the ideal situation would be that they both support this features. The next best thing would be as @xzyfer says and to at least have them be consistent, even if that means neither of them supporting it.

You should be able to switch from libsass to ruby sass in theory without anything breaking. Having something which would knowingly prevent this from happening imo is a bad thing.

@xzyfer
Copy link
Contributor Author

xzyfer commented Nov 13, 2015

You should be able to switch from libsass to ruby sass in theory without anything breaking

Exactly.

The next best thing would be as @xzyfer says and to at least have them be consistent

This is what we will do.

@xzyfer
Copy link
Contributor Author

xzyfer commented Nov 13, 2015

Sass 4.0 will bring in a way to do this properly.

@esr360
Copy link

esr360 commented Nov 13, 2015

Fantastic! Can't wait.

@xzyfer xzyfer modified the milestones: 3.3.3, 3.4 Nov 28, 2015
@mgreter mgreter modified the milestones: 3.3.4, 3.3.3 Jan 9, 2016
@mgreter mgreter modified the milestones: 3.3.5, 3.3.4 Feb 23, 2016
xzyfer added a commit to xzyfer/libsass that referenced this issue Mar 26, 2016
This patch address two inconsistencies with how `@import` is
resolved compared to Ruby Sass.
- `.css` should not be `@import`able
- `.sass` should take precedence over `.scss` files

Fixes sass#1656
@mgreter mgreter modified the milestones: 3.3.6, 3.3.5 Apr 6, 2016
@mgreter mgreter modified the milestones: 3.3.7, 3.3.6 Apr 23, 2016
@mgreter mgreter modified the milestones: 3.3.8, 3.3.7 Apr 30, 2016
@xzyfer xzyfer modified the milestones: 3.4, 3.3.8 May 20, 2016
@xzyfer
Copy link
Contributor Author

xzyfer commented May 21, 2016

@mgreter I've talked to @chriseppstein and @nex3 about this problem. We've decided that even though inline of CSS imports breaks Ruby Sass compatibility in a big way, removing this feature is likely to upset users. This has been a long requested feature in Ruby Sass which wasn't possible due the limitations of being a CSS superset. As a result almost certain people are depending on this behaviour.

Let's instead aim to remove this in 4.0 since users are more accepting of breaking changes in major releases, and because we'll have an official way to inline CSS imports.

@xzyfer xzyfer closed this as completed May 21, 2016
@xzyfer xzyfer removed this from the 3.4 milestone May 21, 2016
@atalis
Copy link

atalis commented Sep 21, 2016

We recently switched to the latest version of Node from a much older version and had to upgrade a 3rd party library we are using, to be compatible with the current Node version. Now things are broken all over the place because of this bug. I understand this issue is not resolved in libsass yet, so I'm trying to figure out recursively what versions of our dependencies we'd need to go back to in order to get rid of this issue.

So, a few questions (apologies, if I missed this info here, this thread is a bit convoluted):

  1. What version of libsass was this bug introduced?
  2. What version is the fix going into - 3.5? 4.0?
  3. When is that version scheduled to release? (Or has it already been released, perhaps?)

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 22, 2016

What version of libsass was this bug introduced?

I can't be certain. I think it was around 3.3.0 which is in [email protected].

What version is the fix going into - 3.5? 4.0?

This will be fixed in 4.0 once @use lands. It will allow people explicitly opt-in to CSS imports.

When is that version scheduled to release?

Impossible to say unfortunately. Not likely to be this year.


As an aside if you're using node-sass you can write a custom importer to noop imports for .css files.

@steveworkman
Copy link

As @xzyfer said, try a custom importer, I had an issue where we had .scss files and .css files in the same directory, and here's the importer I made for node-sass

importer: function(url, prev, done) {
	// Use this to import scss files, instead of css ones
	let updatedUrl = url;
	if (!url.endsWith('.scss') && !url.endsWith('.css')) { // Also don't change ones already explicitly set to .css
		updatedUrl += '.scss';
	}
	grunt.verbose.writeln(`Changed ${url} into ${updatedUrl}`);
	done({file: updatedUrl});
}

Tested with node-sass 4.5.2 libsass 3.5.0-beta2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants