Skip to content

Conversation

@jescalan
Copy link

To be honest I have not tested this yet, but according to stylus it should work. Also wow the way this is added is so janky, there has to be a cleaner way to implement this...

@lucasmazza
Copy link
Contributor

I don't know if they should just be aliases - the code used by sass-rails and sprockets has specific configs for each kind of path that it generates (see https://github.com/sstephenson/sprockets/blob/master/lib/sprockets/sass_functions.rb and https://github.com/sstephenson/sprockets/blob/master/lib/sprockets/context.rb#L230-L272) so maybe its better to port some tests from these libraries to see how the implementation here should work.

@jescalan
Copy link
Author

Yeah I'll have to browse the sprockets codebase a little more and figure out what those passed options are actually doing hah - it's weird that they call asset_path right above, which is a shell method. I guess the answer is somewhere in a different file

@jescalan
Copy link
Author

@jescalan
Copy link
Author

Ok, most recent commit I think should do the trick. If this works out, it would be trivial to add the other helpers, the path_to_asset method was just missing an options hash which gets passed through to sprockets/rails

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This options Hash won't do much effect since this method memoizes the first call into a instance variable, I guess we need to rework that a bit. One option would be to memoize the calls based on the given options on a Hash, like @assets_hashes[options] ||=.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm not entirely sure I understand what that first line is doing. If you could be a little more specific with the fix I might be able to better understand and add this in, or maybe you can make a commit that resolves this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the memoization (@assets_hash ||=) is used so the whole logic is executed once and cached in that instance variable. This works fine if the subsequent method calls are using the same arguments (or none), but that isn't the case anymore.

a_hash = assets_hash(scope)
# on this second call the same result of the previous call will be returned, regardless
# of the different options.
a_hash = assets_hash(scope, type: 'image')

For the options Hash to have any effect this memoization needs to go away or the results should be cached differently.

@planetmcd
Copy link

Hello,
We are hitting a problem that I think is related to this on heroku in production mode.
A stylus line

background-image url('back-icon.png') 

is not generating the right url. It is looking for:

 path=/assets/back-icon.png

instead of

 path=/assets/back-icon-9047f607e759ff3e29e7c38fssgf2hh3s.png

Anything we can do to help test?

@lucasmazza
Copy link
Contributor

@planetmcd a sample app would be awesome. Are you using the branch from @Jenius fork?

@stepankuzmin
Copy link

image-path and image-url still not working

@acrogenesis
Copy link

this is still an issue

@psousa
Copy link

psousa commented Dec 5, 2018

having this would be a plus!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants