sass / libsass

A C/C++ implementation of a Sass compiler
https://sass-lang.com/libsass
Other
4.34k stars 463 forks source link

Custom importers shouldn't shadow relative imports #2483

Open nex3 opened 7 years ago

nex3 commented 7 years ago

Note: I'm moderately certain this is a LibSass issue, but it's possible it's an issue with how Node Sass is using the API; sorry if so.

It looks like LibSass will use custom importers to resolve imports before it will try to resolve them relative to their original location. This is contrary to Ruby Sass's behavior, and it also makes relative imports fundamentally risky, since they may be invalidated by a downstream user's import path.

You can reproduce with this script:

var fs = require('fs');
var sass = require('node-sass');
const { StringDecoder } = require('string_decoder');

fs.writeFileSync("base.scss", '@import "imported"');
fs.writeFileSync("imported.scss", 'a {b: relative}');

var result = sass.renderSync({
    file: "base.scss",
    importer: function(url, prev, done) {
        return {contents: 'a {b: importer}'};
    }
});

console.log(new StringDecoder('utf8').write(result.css));

This should print a {b: relative}, but in fact it prints a {b: importer}. Tested with:

node-sass       4.5.3   (Wrapper)       [JavaScript]
libsass         3.5.0.beta.2    (Sass Compiler) [C/C++]
mgreter commented 7 years ago

This was indeed done deliberately as I assumed that a custom importer should also be able to intercept imports for relative paths to i.e. apply its own preprocessing (if wanted). I wasn't aware that ruby sass does not allow to overrule these imports and it feels a bit counter-intuitive to me, since it would mean that a custom importer is called/not called if a certain file does or does not exist (without any chance to intercept it if it does). My premise was that if you add a custom importer you are in charge, and if a custom importer does not want to handle an import it can return null which will trigger a regular import lookup by libsass. With that in mind one should be able to model the behavior you mention by using one of the new sass(_compiler)_find_(file|include) functions (/docs/api-context.md).

nex3 commented 6 years ago

Ruby and Dart Sass model filesystem imports using importers as well. We treat the import paths option as a shorthand for constructing a filesystem importer for each path. If a user wants total control over how all imports are resolved, they can avoid passing in any import paths and use only custom importers.

mgreter commented 6 years ago

@nex3 could you elaborate what "import paths" means? Is this the same as "include paths"? I still don't understand it exactly. In my implementation every importer may access include paths and act accordingly via various methods. If I understand it correctly you would get the same behavior by checking sass(_compiler)_find_(file|include) first before doing your own logic. IMO my original point still stands, as I understand it that "include/import paths" take precedence over any custom importer then? What I was aiming for is i.e. custom pre-processing of a file that is indeed found as a real file within the "include paths". If the file-importer takes precedence you have no chance to alter that file with custom importers?

Edit: one use case would i.e be to implicitly inline url assets within scss files that are in an "import path"?

nex3 commented 6 years ago

could you elaborate what "import paths" means? Is this the same as "include paths"?

Yes, Node Sass's includePaths option refers to the same thing.

What I was aiming for is i.e. custom pre-processing of a file that is indeed found as a real file within the "include paths". If the file-importer takes precedence you have no chance to alter that file with custom importers?

In Ruby and Dart Sass, import paths are just a special case of importers. If a user wants full control over how all @imports are resolved, they can just avoid passing in include paths at all and leave all resolution—including finding files on the filesystem—up to their custom importer. But this is an edge case. In most cases, authors need to be confident that their relative imports won't get clobbered by some downstream importer, especially since importer authors may not be diligent about ensuring that all their imports are well-namespaced.

just-boris commented 6 years ago

I was migrating my project from node-sass to dart-sass when I encountered an issue caused by the new dart-sass version behavior.

We use custom importer function to rewrite file paths depending of the current ui-theme. We have the following file structure

src/sass/
├── index.scss
├── button.scss
├── button.custom.scss
├── constants.scss
├── constants.custom.scss
└── dropdown.scss

When the "custom" theme is being built, importer enforces .custom.scss files to be included instead of default ones. This way we can override only some parts of the theme without rewriting it all.

With migration to dart-sass, the importer doesn't work for us, because it is not called, as soon as files for original theme are present, so we always get the "original" theme built, even if "custom" was requested.

nex3 commented 6 years ago

@just-boris I suggest having some way of explicitly indicating in the import string that your imports are meant to be overridable. For example, you could have @import "theme:button" know to look for button.custom.scss if the custom theme is being used, and button.scss otherwise. This is also more user-friendly, because it's clear to the reader that something other than normal import semantics are being used.