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

url() with variable or relative path in sass/scss is broken #7651

Closed
sapphi-red opened this issue Apr 8, 2022 · 7 comments · Fixed by #10741
Closed

url() with variable or relative path in sass/scss is broken #7651

sapphi-red opened this issue Apr 8, 2022 · 7 comments · Fixed by #10741
Labels
feat: css p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@sapphi-red
Copy link
Member

sapphi-red commented Apr 8, 2022

This issue organizes many issues around relative url() in sass/scss and adds many information.

(Please convert to discussion if it should be a discussion.)

Related: #6618, #3164, #2993, #5337, #4269

Premise

sass does not support rebasing relative url (sass/libsass#532).
For example,

/* src/foo.scss */
@import "./nested/bar.scss";

/* ---- */
/* src/nested/bar.scss */
.bar {
  background: url('./bar.png'); /* intends nested/bar.png */
}

becomes

/* src/foo.css */
.bar {
  background: url('./bar.png'); /* becomes src/bar.png */
}

Also there is no API for custom rebasing (sass/sass#2535).

In sass-loader, this feature is not implemented. It presents resolve-url-loader for solution. This one rebases url after converted to css and uses sourcemaps to obtain the original file path.

Current Vite's implementation

It is implemented by this rebaseUrls function.
This function is called inside importer option which is passed to sass.
But importer will only be called if it is not a relative import because of the resolve order.

Loads are resolved by trying, in order:

  • Loading a file from disk relative to the file in which the @use or @import appeared.
  • Each custom importer.
  • Loading a file relative to the current working directory.
  • Each load path in includePaths.
  • Each load path specified in the SASS_PATH environment variable, which should be semicolon-separated on Windows and colon-separated elsewhere.

Interface LegacyStringOptions importer

Which means if a file is resolved by relative path, rebaseUrls functions won't be called.
The example is below.

/* src/foo.scss */
@import "./nested/bar.scss";
/* @import "/@/nested/bar.scss"; */ /* if a alias is used `rebaseUrls` will be called */

/* ---- */
/* src/nested/bar.scss */
.bar {
  background: url('./bar.png');
}

This is why #6618 only happened when alias is used. (So url won't be rebased if it is imported by relative path.)

Also rebaseUrls is doing the transform over sass files and this makes it unable to rebase url including variables like url($url) (#3164, #2993, #5337).
This can be mitigated by not transforming url which starts with a variable (#4269).
But it won't work if it is used like below.

$url: './relative.png';

.foo {
  background: url($url);
}

Solutions

I suggest several solutions.

  1. Since this is not a sass's official feature, drop this feature(rebasing relative path) from Vite.
    • Supporting absolute paths (including paths from config.root) is easily achieveable by doing rebaseUrls after the content is transformed to css.
  2. Mitigate as much as possible
    • I have no idea how to call rebaseUrls even if it is imported by relative path.
  3. Implement a logic simillar to resolve-url-loader.
    • Maybe if the sourcemaps are enabled, UrlRewritePostcssPlugin can handle this.
@bluwy bluwy added the feat: css label Apr 9, 2022
@haoqunjiang haoqunjiang added p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Apr 11, 2022
@haoqunjiang haoqunjiang moved this to Discussing in Team Board Apr 11, 2022
@sapphi-red
Copy link
Member Author

The same can be said for less.
But less provides rewriteUrls option which may help implementing much easier than sass.
related: #3644

@yyx990803
Copy link
Member

To be absolutely strict:

  • rewriteCssUrls need to differentiate between quoted and unquoted url() values
  • We also need to pass information on which preprocessor we are dealing with
  • Only skip rewrite if:
    • In sass/scss, unquoted, & starts with $
    • In less, unquoted & starts with @

frankyeyq pushed a commit to frankyeyq/vite that referenced this issue Jun 24, 2022
jarham added a commit to online-inquiry-tool/oit that referenced this issue Aug 25, 2022
Old version kept:
- bootstrap-icons, 1.9 changed scss to use variables in font urls which
  broke compatibility with vite's scss processing. bootstrap-icons seems
  to have valid scss, so the actual problem is with Vite (and sass not
  supporting url rewriting). Importing bootstrap-icons.css could be used
  as a workaround. See:
  - twbs/icons#1381
  - vitejs/vite#7651
  - sass/sass#1015
  - sass/sass#2535
  - sass/sass#2927

Updated only up to minor version:
- @types/node, still using Node 16 as it's the latest LTS at this time
sapphi-red pushed a commit to sapphi-red/vite that referenced this issue Oct 31, 2022
@andyexeter
Copy link

This was closed as completed in #10741 but it has not been fully resolved. Paths with relative assets do not work correctly as described in the original issue description.

@sapphi-red
Copy link
Member Author

@andyexeter Which one are you talking about? (I've described many cases in the original post) Would you create a reproduction for that?

@andyexeter
Copy link

@sapphi-red I was talking about this one:

sass does not support rebasing relative url (sass/libsass#532). For example,

/* src/foo.scss */
@import "./nested/bar.scss";

/* ---- */
/* src/nested/bar.scss */
.bar {
  background: url('./bar.png'); /* intends nested/bar.png */
}

becomes

/* src/foo.css */
.bar {
  background: url('./bar.png'); /* becomes src/bar.png */
}

However, upon trying to create a reproducer it does in fact seem to be working as expected now.

I suspect my issue is being caused by using Vite in a Symfony application with https://github.com/lhapaipai/vite-bundle

Apologies :)

@andyexeter
Copy link

@sapphi-red After some discussions with the maintainer of https://github.com/lhapaipai/vite-bundle, we have discovered that this is actually an issue with Vite itself and is something we have been able to replicate. I have created a reproducer here:

https://github.com/andyexeter/vitejs-vite-2btrkm

The issue is only present when the main.js file and other assets are moved into a top level assets directory as per the above reproducer.

In the reproducer you'll see a assets/styles/pages/home.scss file with the following background-image property:

background-image: url('../../images/vite.svg');

This is correctly referencing the images directory two levels up, however the background image does not appear and the following message is displayed during an npm run build:

../../images/vite.svg referenced in /home/andy/vitejs-vite-2btrkm/assets/styles/style.scss didn't resolve at build time, it will remain unchanged to be resolved at runtime

The only way to get the background image to appear is by changing the path to:

url('../images/vite.svg');

But this is incorrect and not relative to the home.scss file.

Let me know if you want me to create a new issue for this.

@sapphi-red
Copy link
Member Author

@andyexeter I see. It would be great if you create a new issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: css p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
Archived in project
5 participants