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

Digests of files that can have dependencies on other files in the load path need to reflect those dependencies #188

Merged
merged 15 commits into from
May 18, 2024

Conversation

dhh
Copy link
Member

@dhh dhh commented May 17, 2024

CSS files that reference other CSS files via @import or images via url() need to have their digest reflect changes in the dependencies. This PR adds that by including the content of all dependencies in the calculation for such assets with dependencies.

This takes some inspiration from #175 but avoids the dependency tree tracking.

@wlipa
Copy link
Contributor

wlipa commented May 17, 2024

For some reason, I'm not getting digested assets in CSS files with this. I do get digests from the asset helpers though.

@dhh
Copy link
Member Author

dhh commented May 17, 2024

@wlipa What do you mean exactly? That @import url isn't being replaced with a digested url?

@dhh
Copy link
Member Author

dhh commented May 17, 2024

Ah, I see the problem.

@wlipa
Copy link
Contributor

wlipa commented May 17, 2024

In content_with_compile_dependencies, the references will need to be returned in a stable order in order for digests to be stable. Maybe they are if it's based on source code order? Not 100% sure on set merging.

@dhh
Copy link
Member Author

dhh commented May 17, 2024

Should be stable given the scan will always return them in order. But good question about set guaranteeing a stable order. Will investigate.

@dhh
Copy link
Member Author

dhh commented May 17, 2024

As far as I can tell, since Ruby 2.5, the insert order is guaranteed in a set, and that includes merging. So we should be good.

@wlipa
Copy link
Contributor

wlipa commented May 18, 2024

This is checking out for me now. 👍🏼

@dhh
Copy link
Member Author

dhh commented May 18, 2024

Thanks for your help with this, @wlipa. Got some good inspiration for this approach from your work, and I think we ended up in a nice place without complicating the domain model much.

@dhh
Copy link
Member Author

dhh commented May 18, 2024

Fixes #123 and #90. Closes #175.

@dhh dhh merged commit 59406ab into main May 18, 2024
6 checks passed
@wlipa
Copy link
Contributor

wlipa commented May 18, 2024

Yeah sounds good. It's definitely simpler. The only thing I noticed that's a bit of a trade-off is reading all the dependent assets into memory to do that sum. Could be a lot with big images. But maybe it's not that much of a problem in practice.

@dhh
Copy link
Member Author

dhh commented May 18, 2024

Yeah but we read them into memory anyway to digest them individually. Although of course it's perhaps a higher peak if a single css file references a ton of images. Still, doubt it's an issue. Your images shouldn't be that big in the first place if referenced by css!

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.

2 participants