Url() without quotes is not supported in sass

[StarpTech]

According to https://developer.mozilla.org/en-US/docs/Web/CSS/url there are three formats.

div#test {
  background-image: url(./images/test.jpg)
}

{
  "name": "parcel-demo",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "start": "parcel index.html"
  },
  "author": "Dustin Deus",
  "license": "ISC",
  "devDependencies": {
    "autoprefixer": "^7.2.3",
    "babel-preset-env": "^1.6.1",
    "node-sass": "^4.7.2",
    "postcss-modules": "^1.1.0",
    "uglify-es": "^3.2.2"
  }
}

Exptected behaviour

./images/test.jpg should be in dist folder

Current behaviour

./images/test.jpg is not in dist folder

Parcel: 1.2.0
Windows 10

[josephch405]

It seems like the image is actually there - just hashed and put at the root directory. If I'm not mistaken, a key part of using a bundler in the first place is that assets such as images would be processed like so - so instead of having a ./images/test.jpg, the bundler replaces references to it with 7286d30e8328e17dbf8fa60c2c4b1ffb.jpg (as an example). Not sure if actually an issue - we want assets to stay hashed, right?

[davidnagli]

Looks like it works correctly according to @josephch405. Feel free to reopen this if you thinks it’s still an issue :)

[StarpTech]

The issue is not about naming the image is missing completely. The image is only there when I use quotes.

[StarpTech]

Please reopen.

[davidnagli]

@StarpTech Oh I see, so you’re saying that the image doesn’t appear in the dist directory at all?

[StarpTech]

Yes!

[brandon93s]

This is related to sass specifically. This works as expected when using vanilla css, but when converting to sass and adding node-sass the issue is reproduced.

[brandon93s]

This is occurring because the url function isn't executing when files do not have quotes, and therefore are not being registered as dependencies:

parcel/src/assets/SASSAsset.js

Lines 26 to 31 in c008bb0

	opts.functions = Object.assign({}, opts.functions, {
	url: node => {
	let filename = this.addURLDependency(node.getValue());
	return new sass.types.String(`url(${JSON.stringify(filename)})`);
	}
	});

The scss-url test can be used to reproduce this if you update integration/scss-url/index.scss by removing the quotes.

I'm inclined to think this is a bug with node-sass, but I cannot tell if it's a bug or if we should also be implementing an image-url and font-url function(s). I tried this, and they are not called. node-sass notes that their "functions" are still experimental...

Is anyone more familiar with SASS / node-sass able to chime in? We could "fix this" pretty easily with a regex that adds quotes, but that's a hack at best.

[xzyfer]

Hello, I'm the node-sass maintainer. Happy to help whenever I we can regarding Sass issues. I don't fully grok what's happening here, still very new to parcel. Happy to answer any specific questions.

[brandon93s]

Hello @xzyfer, allow me to provide a simplified recap:

// consider the following scss file:
#one {
      background-image: url(./images/test.jpg)
}

#two {
      background-image: url('./images/test.jpg')
}

parcel will parse this using node-sass in roughly the following way:

let style = `
#one {
      background-image: url(./images/one.jpg)
}

#two {
      background-image: url('./images/two.jpg')
}
`;
const nodeSass = require('node-sass');
let opts = {
  data: style,
  functions: {
      url: node => {
		console.log(node.getValue())
      }
  }
}

nodeSass.render(opts);

When this runs, we're seeing './images/two.jpg' trigger the url function, but not ./images/one.jpg (no quotes). parcel is currently using this fn implementation to register dependencies and transform the urls.

Is this the expected behavior?

Thanks!

[brandon93s]

After reading some more... I'm beginning to think the current implementation is just a bad idea. Overriding url, that is. Or at least seems to be historically problematic.

[xzyfer]

Thanks. To clarify these two cases shouldn't act differently, however I'm not sure at the moment what the correct behaviour is. Specifically I'm not sure if url is a function that should be able to be overridden with custom functions because this could have knock on affects to custom importers i.e. @import url('foo').

I'll need to confirm what we expect to happen when I'm back home.

[xzyfer]

My gut feeling is that a url custom function should work, and the working behaviour is a bug. (still need to confirm)

For reference this is some prior work regarding an official implementation on how to do this in sass/libsass#532. It was largely put on ice once https://github.com/sass-eyeglass/eyeglass came up with a solution that was good enough.

Recently there has some been renewed discussion on formalising an approach to support custom url loaders in modern Sass engines, but it's early days still.

[xzyfer]

Out of curiosity how are you doing this with other preprocessors like less?

[brandon93s]

Less supports programatic plugins which makes visiting and updating all urls a bit more trivial:

parcel/src/assets/LESSAsset.js

Lines 31 to 48 in c008bb0

	function urlPlugin(asset) {
	return {
	install: (less, pluginManager) => {
	let visitor = new less.visitors.Visitor({
	visitUrl: (node, args) => {
	node.value.value = asset.addURLDependency(
	node.value.value,
	node.currentFileInfo.filename
	);
	return node;
	}
	});
	visitor.run = visitor.visit;
	pluginManager.addVisitor(visitor);
	}
	};
	}

Internal implementation details here: https://github.com/less/less.js/blob/master/lib/less/visitors/visitor.js

visitUrl will run specifically for Url() nodes.

[xzyfer]

I have confirm url is a reserved function and so should not be overrideable. Future versions of node-sass will patch the current broken behaviour.

[brandon93s]

Future versions of node-sass will patch the current broken behaviour.

Does this mean it will not be overrideable in any capacity in future versions?

[xzyfer]

Not as a custom function. However there will eventually programmatic url loaders. Similar to custom importers.the timeline for this feature is unknown. I do not want to break things for parcel users. I would prefer that url is overrideable always until we have the url loader, if feasible in LibSass.

…

On 19 Dec. 2017 12:45 pm, "Brandon Smith" ***@***.***> wrote: Future versions of node-sass will patch the current broken behaviour. Does this mean it will not be overrideable in any capacity in future versions? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#316 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWAO9A1VsnLNVINFxV01pglUD7N1oks5tBxVJgaJpZM4REpn_> .

[StarpTech]

This statement will remove full-support for SASS in parcel except we will implement a custom url() parser or the loader api in LibSass is ready but I think this can take a while.

[xzyfer]

I'm not quite following what you mean @StarpTech. I'm hoping that by our next node-sass release url will be reliably overridable (resolving this issue).

[StarpTech]

That's great I think I misunderstood your last answer because for me it sounds like that nothing was defined or scheduled.

[TimNZ]

This is still broken.
Any update?

[xzyfer]

I've digging into this from the Sass side of things. As it stands if you want to intercept the url() function call the argument must be a quoted string.

If the string is not quoted in some cases it will be treated a dynamic Sass script and will not be interceptable.

[xzyfer]

There has been some recent discussion on have native url loader within the Sass language but it's still early days.

[TimNZ]

@xzyfer Thanks for the update.

For Parcel team, every common real world scenario that prevents Parcel from working as expected, the ability to use Parcel goes to zero very quickly.

A lot of third party scss has unquoted url()

[jeyj0]

I have created a simple demo-project showcasing this issue to see whether it still exists, and it looks like it does.

I am not sure whether there was a decision made in this thread, but it looks to me like it simply got stale. If it should get fixed inside of parcel, I'd be happy to create a PR if I'd get some pointers as to where I'd have to start. 😄 If it needs to be fixed in another repository, give me a hint and I'll see whether I can do something there.

[DeMoorJasper]

I am not sure whether there was a decision made in this thread, but it looks to me like it simply got stale. If it should get fixed inside of parcel, I'd be happy to create a PR if I'd get some pointers as to where I'd have to start. 😄 If it needs to be fixed in another repository, give me a hint and I'll see whether I can do something there.

Guess you haven't read the thread the issue is inside sass not inside parcel, we can't overwrite something that isn't allowed to be overwritten by sass, we could however ignore sass altogether for urls and pass them to postCSS. Although that might be a bit hacky. We could also Regex it out, but that seems even more hacky

[jeyj0]

Ah, sorry, my bad. I wasn't sure what the final conclusion was - whether it is a parcel issue or sass. Seems I didn't understand it correctly. :)

I'd not "hack" something into parcel if the issue isn't that big. Not many people seem to come here, if the thread has been stale for so long. Thanks though!

[DeMoorJasper]

@jeyj0 no worries, we can still fix it. I've opened a PR which just removes the responsibility from sass and moves it to PostCSS, it might slow down a lil due to recreation of ASTs and such, but at least it works all the time.

See #1909

[StarpTech]

@xzyfer promised a solution here #316 (comment) what's the status?

[jeyj0]

Oh wow, that was fast!

I'd be interested to get to know the Repo better. I'll look further into it tomorrow, but I currently don't see where it's handed over to PostCSS in the commit? I see that it isn't handled by sass anymore, due to the removal of the big block. Is it then just handled by PostCSS automatically? I'll look longer tomorrow, but I'd really love a little explanation to just understand a bit of parcel's code, to hopefully start contributing at some point. :)

[DeMoorJasper]

@jeyj0 Sure, so Parcel uses what we called the processing pipeline (it will probably get a better name in Parcel 2).

You can see the pipeline's processAsset code here: https://github.com/parcel-bundler/parcel/blob/master/src/Pipeline.js#L36 (it gets called from the worker)

What it does is it takes an asset which has a type (in this case sass) processes it through the SASSAsset transform. That transform responds with a css asset, which the pipeline than puts through the CSSAsset transform. This keeps going untill it has a final asset type (originalType === result basically).

Here are some examples
SASS => CSS
Pug => HTML
TS => Babel => JS

In Parcel 2, the Pipeline (or whatever we wanna call it) is gonna be less magical and more clear to understand (and fully configurable), but I hope my explanation got you a basic understanding of what it does :)

[jeyj0]

Thanks a lot! I'll look into the code some more the next days! 😄

[xzyfer]

Apologies I thought I had responded here. I looked into a solution in Sass but it's not possible without breaking language compatibility.

There is an issue to track official support for this feature: sass/dart-sass#195