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

Do not digest already digested files #10

Merged
merged 1 commit into from
Oct 9, 2021

Conversation

brenogazzola
Copy link
Collaborator

@dhh Not sure how you will prefer to handle this one:

If I have an already digested file like trix_chunk-abcdef012345678.js in /build, then Asset will use the entire thing as the logical_path, but Server will only pass to LoadPath the string trix_chunk.js. Either Asset or LoadPath have to strip the digest too. Which do you prefer?

@dhh
Copy link
Member

dhh commented Oct 7, 2021

I'm concerned that -([0-9a-f]{7,128}) will pickup things like javascript-somethingabovesevencharacters.js and not digest. We actually have the same problem with the change made to Sprockets. I think it's good we didn't do a release there yet!

@brenogazzola
Copy link
Collaborator Author

Ok. How about instead of detecting the digest we detect a keyword in the name? Since we can control the file name through webpack, we could tell it to add something like -digested before or after the content hash and sprockets/propshaft would use that to skip digesting.

@dhh
Copy link
Member

dhh commented Oct 7, 2021

Yes, I think that's safer. Maybe something like lib-aw68as6wewq.digested.js?

@brenogazzola
Copy link
Collaborator Author

Ok, I'll submit a PR for Sprockets too.

@brenogazzola
Copy link
Collaborator Author

Did you mean we should detect .digested.js or -[digest].digested.js? 😅

@dhh
Copy link
Member

dhh commented Oct 7, 2021

I guess -[digest].digested.js is even safer. Highly unlikely to snare files you didn't intend to catch with this.

@brenogazzola brenogazzola force-pushed the skip-already-digested branch from 3a7fd60 to 03297de Compare October 7, 2021 14:13
@brenogazzola
Copy link
Collaborator Author

Ok, done here. Will go back to jsbundling and sprockets to adjust. We still need the problem with Server stripping the digest. Guess that now that we have a keyword we can detect that and skip?

-- digest    = full_path[/-([0-9a-f]{7,128})\.[^.]+\z/, 1]
++ digest    = full_path[/-([0-9a-f]{7,128})\.(?!digested)[^.]+\z, 1]

@dhh
Copy link
Member

dhh commented Oct 9, 2021

Yes, can you do a PR with test for the server change?

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