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

Gracefully handle missing fingerprinted file #334

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

herschel666
Copy link

Hi.

As we've briefly talked about already in Slack, I'm not very fond of the way Architect handles requests to fingerprinted files, that aren't present in the manifest.json's mapping. In case the file could not be found, having the HTML resource, that initiated the request to the static asset, fail with a 500, feels to me like shooting the messenger. Imho only the request of the missing static asset itself should fail — with a 404. The change I made in this PR introduces exactly that.

A drawback of this is, that missing static assets aren't that obvious anymore. On the other hand, this is not uncommon. Hunting down subsequent 404s after page load requires a little effort. I think that's fine.

What do you think?

@brianleroux
Copy link
Member

thought about this for a while and I agree; this would be a better behavior… also a breaking change I think so I'll poll other maintainer opinions here. thx for your patience! cc @ryanblock @filmaj

@ryanblock
Copy link
Member

ryanblock commented Mar 26, 2024

Just triaging some old PRs here! There's some overlapping abstractions here that may be confusing the core issue – let's separate HTTP error codes from the method behavior.

Ultimately this method exists to map non-fingerprinted to fingerprinted filenames. If called with a file that doesn't exist, I don't think it should act as though it exists, and pass back a value that may or may not work. I can get there that perhaps throwing is too aggressive, so maybe we should instead just return undefined or null?

@herschel666
Copy link
Author

[…] so maybe we should instead just return undefined or null?

That would also be fine.

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

Successfully merging this pull request may close these issues.

3 participants