-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add Propshaft::Compiler::JsAssetUrls #207
Add Propshaft::Compiler::JsAssetUrls #207
Conversation
a28e06c
to
f840ca1
Compare
This is very interesting. I hadn't considered this path. And I guess partly because I was thinking we needed some actual JS code to do the lookup. So I like where this is going, but I wonder if a fake method is the right way to go. If it is, I'd like to call it out as much as possible, so there's a clear clue that this is actually a compiler macro, not a real method. Maybe we could do something like: export default class extends Controller {
init() {
this.img = RAILS_ASSET_URL("/foobar/source/file.svg");
}
} |
f840ca1
to
699be47
Compare
I hadn't considered simply making the function call look different, but I was concerned about potential collisions with other libraries which lead me to adding the "rails" prefix. I like the compiler macro approach 👍 I've also added this to the default compiler setup in the railtie, but can completely understand if you'd rather I remove that. |
What I like about the macro-looking function is that it should still pass JS linting, because you CAN have capitalized methods in JavaScript. It just obviously isn't the common approach. Yeah I think this is solid 👌 |
@brenogazzola You have any thoughts on this? I know we've been wrestling with this issue for quite a long time. I like what I'm seeing here, but wanted your opinion on it too. |
Well, the macro is similar to the asset helpers that Sprockets used for so long in CSS files, so we know it works and people upgrading from it won't find it strange. I just wish there was a way to do this without needing the macro. It feels odd explaining that CSS handles the URLs automatically but Javascript files need the extra code. Is it mostly images we are talking about? Can't we just look for |
Yeah, I think it's too dangerous just to let a regexp loose on JS code searching for what might be a path. Fully agree that it's much nicer when it can just be invisible, as with CSS. But I don't see a path to doing that with JS. The other option was indeed to add a JS library that could do lookups in a manifest or something. I don't think that's better though. |
I had been hoping that if we could figure something here, we could later do something similar for package imports/chunking so we could remove the Anyway, the only downside I can see with REGEX based solutions for Javascript is that they can't handle interpolated strings, but those should be easy enough for the devs to code around, even if it might make their code less nice. So yeah, I don't see a problem with this being merged. |
@brenogazzola Could you test this on your end too? Just making sure that we haven't tripped anything obvious that breaks. I'll do the same in HEY. |
Sure |
Tested in HEY. Looking good. We should have some documentation for this, btw. |
Looks fine here in Festalab too. |
Rocking. Shipped in 1.1.0. |
I’m a bit confused why this is needed. In my codebase, we just pass the asset urls through to JS as HTML data attributes, or via JS (or JSON) objects similar to importmap. It feels overcomplicated to manipulate the raw JS, and seems like the start of a slippery slope to a more complex build process. I think treating URLs as data, just like importmap does, is a good way to decouple compiled assets from actualy code. Here's an example of how we do this using Stimulus attributes:
And here's an example of how it could be done outside Stimulus:
|
I'm a fan of it. Sometimes there's an asset that really is only used from JS, and it's just nicer to maintain that reference in one place. It's not "needed" because you can work around it, but it makes things simpler for developing imho. |
@dkniffin When you have control to do that, that's great. But I also think this is perfectly reasonable and inline with the digest stamping we do for CSS files. Propshaft's mission is to do two things: Provide a load path and offer asset digest stamping. We should do that the best we can, but then deny any further scope creep. |
Not sure if this would just be better as a gem if the use case is simply not common enough, but I thought I'd see.
I recently found myself working on some javascript that needed to be able to reference a variety of images from our assets, but couldn't reliably do that because of the asset digest change to the filename. The solution for handling this in CSS works great and I wanted something similar for our javascript.
This compiler is almost 100% the same as the css asset compiler with the only real changes being to the regex and how assets that aren't in the load path are handled.