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

Add Propshaft::Compiler::JsAssetUrls #207

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

keiththomps
Copy link
Contributor

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.

@keiththomps keiththomps force-pushed the keiththomps/add-js-asset-urls-compiler branch 2 times, most recently from a28e06c to f840ca1 Compare September 27, 2024 14:16
@dhh
Copy link
Member

dhh commented Sep 29, 2024

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");
            }
          }

@keiththomps keiththomps force-pushed the keiththomps/add-js-asset-urls-compiler branch from f840ca1 to 699be47 Compare September 30, 2024 12:20
@keiththomps
Copy link
Contributor Author

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.

@dhh
Copy link
Member

dhh commented Sep 30, 2024

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 👌

@dhh
Copy link
Member

dhh commented Sep 30, 2024

@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.

@brenogazzola
Copy link
Collaborator

brenogazzola commented Sep 30, 2024

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 .png, .jpg, .svg at the end of strings in Javascript files? I don't write enough javascript to know if this would interfere with something else though.

@dhh
Copy link
Member

dhh commented Sep 30, 2024

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.

@brenogazzola
Copy link
Collaborator

brenogazzola commented Sep 30, 2024

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 .digested thing we have, which keeps messing with the code.

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.

@dhh dhh merged commit e28a762 into rails:main Sep 30, 2024
3 checks passed
@dhh
Copy link
Member

dhh commented Sep 30, 2024

@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.

@brenogazzola
Copy link
Collaborator

Sure

@dhh
Copy link
Member

dhh commented Sep 30, 2024

Tested in HEY. Looking good.

We should have some documentation for this, btw.

@brenogazzola
Copy link
Collaborator

Looks fine here in Festalab too.

@dhh
Copy link
Member

dhh commented Sep 30, 2024

Rocking. Shipped in 1.1.0.

@keiththomps keiththomps deleted the keiththomps/add-js-asset-urls-compiler branch September 30, 2024 23:34
@dkniffin
Copy link

dkniffin commented Oct 3, 2024

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:

{
  "data-controller": "foo",
  "data-foo-bar-url-value": asset_path("bar.png")
}

And here's an example of how it could be done outside Stimulus:

# In ruby somewhere:
<script>
  RAILS_ASSET_URLS = {
    "bar.png": #{asset_path("bar.png")}
  }
</script>

// Then to use it in JS somewhere:
RAILS_ASSET_URLS["bar.png"]

@wlipa
Copy link
Contributor

wlipa commented Oct 3, 2024

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.

@dhh
Copy link
Member

dhh commented Oct 3, 2024

@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.

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.

5 participants