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

Should libsass have an image-url function? #489

Closed
michaek opened this issue Sep 29, 2014 · 37 comments · Fixed by #834
Closed

Should libsass have an image-url function? #489

michaek opened this issue Sep 29, 2014 · 37 comments · Fixed by #834

Comments

@michaek
Copy link
Contributor

michaek commented Sep 29, 2014

See

Signature image_url_sig = "image-url($path, $only-path: false, $cache-buster: false)";

Because image-url is a Compass function, not a Sass function, it seems odd for it to be included in libsass. Building in any URL functions requires "Sass" to know about your paths - Compass has embraced that pattern, but I don't know if we should assume that Sass does.

@HamptonMakes
Copy link
Member

Hmm... yeah, I don't think we can properly implement URL parsing in C++... without incurring a lot of overhead. I think host languages should be able to include it when we get custom functions in.

@nschonni
Copy link
Collaborator

nschonni commented Oct 3, 2014

👍 the longer this stays in the libarary, the more people that will start using it and making it more painful to remove from the API

@HamptonMakes HamptonMakes added this to the 3.0 milestone Oct 3, 2014
@akhleung
Copy link

akhleung commented Oct 3, 2014

Alas, my employer (who sponsored the initial development of LibSass) needs it, which is why it's in there....

@HamptonMakes
Copy link
Member

@akhleung couldn't you use the custom function compilation interface to implement this just for the moovweb gosass?

@akhleung
Copy link

akhleung commented Oct 3, 2014

@hcatlin Yeah, that would be the proper thing to do ... image-url was added early in LibSass's life cycle because we were using it all over the place.

@nschonni
Copy link
Collaborator

nschonni commented Oct 9, 2014

@hcatlin I think this needs to go in the 3.0 release since it is a breaking API change

nschonni added a commit to nschonni/libsass that referenced this issue Oct 9, 2014
This function doesn't currently exist in the Sass spec.
Fixes sassgh-489
@HamptonMakes
Copy link
Member

hmmm…. can we come up with the hardcoded-sass function that is similar? I want to make sure that we don’t alienate people who are using it and ruin their “WEEE! FUN!” feeling of getting all these new features.

@nschonni
Copy link
Collaborator

Yeah, the PR I threw over the wall is the nuclear option.
I nicer in between option would be to keep the image_path option, but it have it spit out a message like

image_path has been removed from libsass till it becomes part of the Sass spec. Till then you can add this snippet to a mixin sheet
@function image-url($url) {
// basic function that returns the correct url
}

I'm suggesting that we actually add the guts to the function, but my brain isn't fully cafinated yet.

@HamptonMakes
Copy link
Member

Yeah, I think that might be the best option for now.

@am11
Copy link
Contributor

am11 commented Oct 11, 2014

@hcatlin, on a slightly related note; is there a way to get CSS output with correct (relative) URL paths? (sass/node-sass#430)

For instance:

foo {
    + bar {
        color: lime;
        background-image: url('../images/7up-fido-dido.png');
    }
}

will produce:

foo + bar {
  color: lime;
  background-image: url(../images/7up-fido-dido.png);
}

When both input and output files are in same directories. But if they are in separate directories, the generated CSS still has the same url. Going by the above example, if input.scss is located at /tmp/input.scss and output path is at /tmp/output/style.css, the expected url value in resultant output is ../../images/7up-fido-dido.png.

If the current behavior is in accord with the sass-specs, it would be nice to have this feature in libsass (and corresponding flag bool resolve_urls (default: false) in the interface) which would do the following:

  • resolve absolute path of asset (in url(..)) w.r.t. input file path (the actual source file; in case of @imports).
  • using that abs. path; resolve relative path of asset w.r.t output file path and use that one in the output.

This way, users won't have to worry about the location of output path, libsass will fix the URLs for them.

@HamptonMakes
Copy link
Member

Yeah, that's a good point @am11. How does Ruby Sass handle this?

@HamptonMakes
Copy link
Member

Can you open a new issue for that discussion, though?

nschonni added a commit to nschonni/libsass that referenced this issue Oct 12, 2014
This function doesn't currently exist in the Sass spec.
Fixes sassgh-489
@am11
Copy link
Contributor

am11 commented Oct 12, 2014

Opened a separate issue here: #532. 😄

@xzyfer
Copy link
Contributor

xzyfer commented Nov 19, 2014

Another thing to consider is that with the addition of function-exists in Ruby sass 3.3 it's now possible to do something like function-exists("image-url").

We do this in our framework to determine whether we can rely on the compass' image-url function or use our own Sass one. The issue we're facing now is that Libsass' native image-url is making this check return true, causing all our image urls to be incorrect (because it's using neither the compass or our sass image url path).

IMHO image-url should be removed. A sass implementation is entire possible by adding the image path to a sass variable.

@mgreter
Copy link
Contributor

mgreter commented Dec 11, 2014

What is the status on this? Would a sass implementation suffice? As I'm thinking, it might be a good idea to add a function to the C-API to set/get variables from the context? That way we could add the image path @xzyfer mentioned.

@xzyfer
Copy link
Contributor

xzyfer commented Dec 11, 2014

IMO this has to be removed as per our roadmap to reach Sass feature parity.

The concerns around breaking the behaviour for people who rely on this are legitimate. @mgreter is the custom function interface capable enough that this feature can be implemented as-is as a custom C function?

@mgreter
Copy link
Contributor

mgreter commented Dec 11, 2014

Custom C functions currently lack the possibility to be called by "mapped" arguments. So I guess this could be a drawback. Other than that I don't see any reason this should not be possible to implement by custom C functions. Although I guess the current code is in C++ (function.cpp)!?

@xzyfer xzyfer modified the milestones: 3.2, 3.1 Dec 21, 2014
@xzyfer
Copy link
Contributor

xzyfer commented Jan 2, 2015

Unless there are any objections I'm going to make image-url produce a deprecation warning in 3.1.1. Then let's figure out what we are going to do about it for 3.2.

@mgreter
Copy link
Contributor

mgreter commented Jan 3, 2015

I'm all in for this! Maybe @hcatlin can pull some strings to get this finally done!? These features should be possible to implement via custom functions now IMO!? I can offer my help to give advice on how to do it via custom functions! But we'd need someone who actually uses this feature to work with us!

@HamptonMakes
Copy link
Member

Hey @chriseppstein, has the compass team gotten to a pure-Sass image-url() implementation yet?

@chriseppstein
Copy link
Contributor

No sass-based definition of image-url in compass since compass requires ruby, there's no point. Any given project can probably implement a trivial image-url() helper for that project.

@HamptonMakes
Copy link
Member

From @jonsuh and @roytomeij:

$image-url-path: “../images/” !default;
@function image-url($url) {
  @return url("#{$image-url-path}#{$url}");
}

@HamptonMakes
Copy link
Member

@chriseppstein Sorry, I thought there was an effort to port some functions from Ruby to Sass, and didn't know if this was one of them.
@xzyfer I think I've agreed since the top to remove it...

@chriseppstein
Copy link
Contributor

@hcatlin There is. This will be one of them :)

I wouldn't say this though:

image_path has been removed from libsass till it becomes part of the Sass spec.

Because it's not something that Sass itself would ever add. Maybe this:

image_path has been removed from libsass because it's not part of the Sass spec.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 5, 2015

Ok cool. I'm going to remove the guts of this and add a deprecation message. 3.2. Is pretty close atm so this is a good a time as any.

@adyballa
Copy link

adyballa commented May 1, 2015

@hcatlin
btw
I have good use of image_path, because I dont know the prefix in sass. So this function will not work

@callumacrae
Copy link

I'm confused. Was this deprecated or removed?

Updating to node-sass 3.1 broke all the images on our website. This is from the output CSS:

.sun-rays {
  background-image: image-url("bg-with-sunrays-large.png");
  background-size: cover;
  background-repeat: no-repeat; }

I'm not getting an error using node-sass through gulp-sass. It's just outputting image-url and silently breaking.

Could I suggest that breaking changes not be hidden at the bottom of the release notes? Also, could you collaborate with the node-sass folks so that breaking changes in libsass are mentioned there, too? There was nothing in the node-sass 3.1 release notes about this function being removed. I don't think most people who use node-sass read the libsass release notes!

Also, in the future, a soft deprecation a version ahead of the head deprecation would be massively appreciated! I had no idea this feature was non-standard until it disappeared.

@xzyfer
Copy link
Contributor

xzyfer commented May 18, 2015

Hi @callumacrae, we work very closely with the node-sass team. I believe this change was clearly communicated, and there were several beta releases.

screen shot 2015-05-18 at 8 56 25 pm

As it stands Libsass does not follow semver, and will not until 3.4. We did soft deprecate this feature in 3.2.0 and subsequently removed in a later patch since the deprecation was causing more troubles to users.

@xzyfer
Copy link
Contributor

xzyfer commented May 18, 2015

Lets consolidate any remaining discussion in sass/node-sass#965.

@danielrz
Copy link

what about the $cache-buster parameter? does LibSass supports cache busting with images?

@xzyfer
Copy link
Contributor

xzyfer commented May 26, 2015

No it does not.
On 26 May 2015 08:37, "danielrz" notifications@github.com wrote:

what about the $cache-buster parameter? does LibSass supports cache
busting with images?


Reply to this email directly or view it on GitHub
#489 (comment).

@swarajban
Copy link

@callumacrae we're having the same problem after upgrading. What did you do to fix?

@xzyfer
Copy link
Contributor

xzyfer commented Oct 17, 2015

You need to implement your own image-url function. It's very straight forward to do.

@idflood
Copy link

idflood commented Oct 17, 2015

@swarajban here is a simple implementation:

sass({
  functions: {
    'image-url($img)': function(img) {
      return new sass.types.String('url("images_directory/' + img.getValue() + '")');
    }
  }
})

@swarajban
Copy link

Thanks @xzyfer and @idflood. I ended up using asset functions from this node package. My CSS image URL's are being set properly again

@GuyPaddock
Copy link

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