Proposal for `url(...)` rewriting #2927

DeMoorJasper · 2020-10-19T08:43:22Z

This PR includes a proposal on how to handle rewriting url's as discussed in issue #2535 for some time now.

Hopefully this helps move things forward a bit on this issue, any feedback is very welcome.

If there's anything else I can do to help this move forward feel free to ask.

proposal/custom-url-resolution.md

DeMoorJasper · 2020-10-21T17:05:04Z

@nex3 thanks for reviewing this proposal, I've changed it quite substantially based on your feedback.

Removed the ability to have multiple plugins in a row as it didn't really make sense after seeing your comment and thinking about it a bit more.
Also made the rewriteUrl plugin also responsible of resolving as you mentioned the importer cannot currently handle this.

Let me know if you think any other things need to change or if there's anything else I can help to move this forward.

nex3

I was reading over #2535 again, and #2535 (comment) in particular where I outlined the general approach the Sass core team had decided on for this. The approach you're describing here differs substantially from the one that comment describes. What's the reasoning behind that difference?

proposal/custom-url-resolution.md

DeMoorJasper · 2020-10-22T06:52:08Z

@nex3 originally I created a proposal based on that comment with the only real difference being using url instead of sass-url but as it would still be blocked by importer as you already mentioned the import api would also need to be changed.

I figured this approach would probably be easier to implement for now and probably solve the problem just as good for most people.

If you think this is a bad idea I could always change this to use sass-url again, that's the only real difference, besides the importer which can always be updated later.

I was also thinking about only rewriting url's that start with file:// based on your comment, which I think is a wonderful idea, just not sure if this is something that should be implemented in here or be a choice for the plugins.

nex3

Sorry for the long delay, I missed your response. For future reference, if you click the icon under reviewers, it will let your reviewer know the PR is ready for another look.

proposal/custom-url-resolution.md

DeMoorJasper · 2020-11-12T16:36:26Z

Thanks for the review @nex3

I hope I've resolved all remarks you had, if I didn't feel free to leave some more feedback.

I've also been thinking about standardising the way this gets configured, as in there should probably be a list of presets that users can utilise (inline, relative, ...). The plugin that is currently introduced in this draft is definitely still required as bundlers will probably want to overwrite the default behaviour regardless to keep track of included files. (I know we do this in Parcel for other css related languages). But a standardised way of configuring this would make it possible for users to switch between webpack, parcel and just the standard sass cli with minimal changes, not sure if you think this should be part of sass or if this is something for the future?

nex3

I think standardizing a few resolvers makes sense. These should probably be exposed as standard resolver functions, or for resolvers that need extra information like relative, a higher-order function that returns a resolver.

proposal/custom-url-resolution.md

…the cli?

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

stof · 2022-09-17T16:15:44Z

proposal/custom-url-resolution.md

+```TypeScript
+let sassOptions = {
+  // Rewrite urls asynchronously
+  rewriteUrl: async (urlReference: { file: string, url: string }, done: (error: Error | null, url: string | null) => void): void => {


Passing a done callback does not make sense to me. The result should be returned in the promise instead (an async function already returns a promise anyway)

Was just mimicing existing sass api's but will update

stof · 2022-09-17T16:16:58Z

proposal/custom-url-resolution.md

+}
+```
+
+### Using Variables


This section is totally unnecessary given that sass-url is defined as a function and not as a custom language construct. All functions are working with the value of their argument without having any way to know how it was produced.

It was just to highlight another use-case or the possibility of this. But I guess that's better left for docs, will remove this

stof · 2022-09-17T16:17:49Z

proposal/custom-url-resolution.md

+import * as sass from 'sass';
+
+const options = {
+  rewriteUrl: sass.url.inline()


should be rewriteUrl: sass.url.inline. We want to pass the function as the value of the option, not calling it.
There is no need to make it a function returning another function IMO.

Right, will update

nex3 · 2023-07-17T18:27:58Z

Since this has had outstanding comments from @stof for several months, I'm going to close it out for now. I'm happy to re-open if @DeMoorJasper wants to come back to it in the future, and I'm also happy to review another PR if someone else wants to take this on.

DeMoorJasper · 2023-07-17T19:10:15Z

@nex3 sorry totally missed those comments, probably got lost in notifications as I usually just see a lot of "bump" responses to the linked issue instead of any feedback here.

But fixed the feedback on my fork, lemme know if I need to do something to get this moving forward again

nex3

I've added a few comments. These should be plenty to work on for now; I'll take a look again once you've addressed them.

nex3 · 2023-07-18T22:58:39Z

proposal/custom-url-resolution.md

@@ -0,0 +1,230 @@
+# Custom url resolution: Draft 1


Style nit

Suggested change

# Custom url resolution: Draft 1

# Custom URL Resolution: Draft 1

nex3 · 2023-07-18T22:59:49Z

proposal/custom-url-resolution.md

@@ -0,0 +1,230 @@
+# Custom url resolution: Draft 1
+
+_[(Issue)](https://github.com/sass/sass/issues/2535)_


Style nit:

Suggested change

_[(Issue)](https://github.com/sass/sass/issues/2535)_

*([Issue](https://github.com/sass/sass/issues/2535))*

This matches the style of other proposals.

nex3 · 2023-07-18T23:00:01Z

proposal/custom-url-resolution.md

+
+> This section is non-normative.
+
+Many css features require the use of a sass-url reference to reference resources from outside the sass files, however these files also need to exist on the eventual output directory and server. To ensure the references are valid, the sass API should allow for the user to provide a way to remap and/or inline these resources.


Please wrap lines at 80 characters.

nex3 · 2023-07-18T23:00:24Z

proposal/custom-url-resolution.md

+
+> This section is non-normative.
+
+Many css features require the use of a sass-url reference to reference resources from outside the sass files, however these files also need to exist on the eventual output directory and server. To ensure the references are valid, the sass API should allow for the user to provide a way to remap and/or inline these resources.


Please use proper capitalization: "CSS", "Sass", etc. Also, code-quote any use of Sass or CSS identifiers or syntax such as sass-url.

nex3 · 2023-07-18T23:03:50Z

proposal/custom-url-resolution.md

+
+> This section is non-normative.
+
+Many css features require the use of a sass-url reference to reference resources from outside the sass files, however these files also need to exist on the eventual output directory and server. To ensure the references are valid, the sass API should allow for the user to provide a way to remap and/or inline these resources.


The background section is supposed to describe the world as it exists today to make the case for a new feature. Saying that we "require the use of a sass-url reference" doesn't make sense, because such a thing doesn't exist currently. What is the problem that exists and that needs to be solved?

nex3 · 2023-07-18T23:08:18Z

proposal/custom-url-resolution.md

+
+> This section is non-normative.
+
+This proposal defines a standardized way to remap the sass-url references to the final location on the server or inline reference.


Again, you're saying "the sass-url references" without explaining at all what sass-url is or does.

nex3 · 2023-07-18T23:09:20Z

proposal/custom-url-resolution.md

+
+This proposal defines a standardized way to remap the sass-url references to the final location on the server or inline reference.
+
+This is accomplished by running sass-url references through the url rewriting plugin if a url rewriting plugin has been defined.


What is "a URL-rewriting plugin"? Where and how are they defined? Are there any built-in ones?

This is just the summary so it doesn't need to be meticulously precise, but this is extremely vague. You need to give a reader a sense of the specifics of your proposal so that they can evaluate it without needing to read the entire spec, not just say "here's the problem, this proposal will solve it". Think of this as a draft of the user-facing documentation that will explain how to use the new feature.

nex3 · 2023-07-18T23:12:59Z

proposal/custom-url-resolution.md

+Whenever a sass-url reference is encountered in a sass file the following steps should be executed:
+
+- A sass-url reference is encountered in the sass file
+- From the url function we extract the parameter value
+- Rewrite the parameter value if it contains any variables ([see using variables](#using-variables))
+- Pass the parameter value to the url rewrite plugin along with the canonical url of the current sass file
+- This plugin than returns a string or calls the done callback with a string value, if it returns anything else or nothing it should throw an error ([see JavaScript API](#javascript-api))
+- The new url value that we received from the url rewrite plugin than gets used to replace the original url value


This is not written in a standard spec style. I recommend looking at other function definitions under spec/ and accepted/ to get a better idea of how to write this.

nex3 · 2023-07-18T23:14:29Z

proposal/custom-url-resolution.md

+
+## Function
+
+This proposal introduces logic for a new function: `sass-url("...")`, this function allow users to define a url that gets rewritten based on the config to ensure it can be loaded correctly from the browser.


This is the sort of thing that would be better in the summary. It does introduce the concept of a sass-url function (that's good!), but it's mostly just a nonspecific explanation of its general purpose. It's also not clear what "the config" refers to here.

DeMoorJasper added 17 commits September 16, 2020 20:15

start work on url-rewrite-proposal

582f243

another edge case

b1147d3

tweak

e5bdaad

api changes for error throwing

0b147a2

more tweaks

6a8454f

more tweaks to api

1cec0e7

add syntax section

3f6e7c0

mention unknown filepaths

668a98b

tweaks

1804a83

remove cli part

918a936

fix

4cbb0af

tweak

9eec6d6

finish initial draft

1e91dae

fix exampkle

a6e5125

remove just

e2e0ac6

add sync example

402c3df

tweak

f631b12

nex3 requested changes Oct 20, 2020

View reviewed changes

DeMoorJasper added 3 commits October 21, 2020 18:02

rewrite proposal based on feedback

1875448

fix wording

b1b6a7c

fix typo

2ff37f0

nex3 reviewed Oct 21, 2020

View reviewed changes

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

tweaks

0fdf1b0

nex3 requested changes Nov 11, 2020

View reviewed changes

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

proposal/custom-url-resolution.md Show resolved Hide resolved

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

DeMoorJasper added 4 commits November 12, 2020 17:08

tweak JS api examples

166146b

rename overload => new behaviour

551d16f

refactor structure

b3257b5

refactor steps and add example in intro

b46f00d

minor tweak

0908b48

DeMoorJasper requested a review from nex3 November 12, 2020 16:29

nex3 requested changes Nov 14, 2020

View reviewed changes

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

DeMoorJasper added 3 commits November 19, 2020 21:04

use sass-url

e2fe419

add the beginning of a preset spec

51c424c

tweak preset writing a bit

af12681

nex3 requested changes Nov 21, 2020

View reviewed changes

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

proposal/custom-url-resolution.md Outdated Show resolved Hide resolved

DeMoorJasper added 9 commits November 21, 2020 13:19

rename url reference to sass-url reference

a1443aa

rename syntax -> function

2a3b47c

remove presets as these aren't really required for now, except maybe …

1f915e9

…the cli?

add inline preset

4300b12

start on relative preset

098f892

fix some bugs in the proposal and finish up relative

29a7c15

tweak

1e71f91

tweak

c3cd0bc

some more small tweaks

39e488f

DeMoorJasper requested a review from nex3 November 26, 2020 13:30

stof reviewed Sep 17, 2022

View reviewed changes

nex3 mentioned this pull request Jun 23, 2023

Define a standard for custom URL loaders #2535

Open

2 tasks

nex3 closed this Jul 17, 2023

Fix changes as requested by Stof

50dbdaa

nex3 reopened this Jul 17, 2023

nex3 requested changes Jul 18, 2023

View reviewed changes

nex3 force-pushed the main branch from c492e66 to e27547c Compare March 27, 2024 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for `url(...)` rewriting #2927

Proposal for `url(...)` rewriting #2927

DeMoorJasper commented Oct 19, 2020

DeMoorJasper commented Oct 21, 2020

nex3 left a comment

DeMoorJasper commented Oct 22, 2020

nex3 left a comment

DeMoorJasper commented Nov 12, 2020 •

edited

nex3 left a comment

stof Sep 17, 2022

DeMoorJasper Jul 17, 2023

stof Sep 17, 2022

DeMoorJasper Jul 17, 2023

stof Sep 17, 2022

DeMoorJasper Jul 17, 2023

nex3 commented Jul 17, 2023

DeMoorJasper commented Jul 17, 2023

nex3 left a comment

nex3 Jul 18, 2023

nex3 Jul 18, 2023

nex3 Jul 18, 2023

nex3 Jul 18, 2023

nex3 Jul 18, 2023

nex3 Jul 18, 2023

nex3 Jul 18, 2023

nex3 Jul 18, 2023

nex3 Jul 18, 2023

	# Custom url resolution: Draft 1
	# Custom URL Resolution: Draft 1

		@@ -0,0 +1,230 @@
		# Custom url resolution: Draft 1

		_[(Issue)](https://github.com/sass/sass/issues/2535)_

	_[(Issue)](https://github.com/sass/sass/issues/2535)_
	([Issue](https://github.com/sass/sass/issues/2535))


		> This section is non-normative.

		Many css features require the use of a sass-url reference to reference resources from outside the sass files, however these files also need to exist on the eventual output directory and server. To ensure the references are valid, the sass API should allow for the user to provide a way to remap and/or inline these resources.


		> This section is non-normative.

		This proposal defines a standardized way to remap the sass-url references to the final location on the server or inline reference.


		This proposal defines a standardized way to remap the sass-url references to the final location on the server or inline reference.

		This is accomplished by running sass-url references through the url rewriting plugin if a url rewriting plugin has been defined.


		## Function

		This proposal introduces logic for a new function: `sass-url("...")`, this function allow users to define a url that gets rewritten based on the config to ensure it can be loaded correctly from the browser.

Proposal for url(...) rewriting #2927

Are you sure you want to change the base?

Proposal for url(...) rewriting #2927

Conversation

DeMoorJasper commented Oct 19, 2020

DeMoorJasper commented Oct 21, 2020

nex3 left a comment

Choose a reason for hiding this comment

DeMoorJasper commented Oct 22, 2020

nex3 left a comment

Choose a reason for hiding this comment

DeMoorJasper commented Nov 12, 2020 • edited

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 commented Jul 17, 2023

DeMoorJasper commented Jul 17, 2023

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Proposal for `url(...)` rewriting #2927

Proposal for `url(...)` rewriting #2927

DeMoorJasper commented Nov 12, 2020 •

edited