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

[✨] Automatic simple 404 responses for file requests #6302

Closed
intellix opened this issue May 12, 2024 · 9 comments · Fixed by #6341
Closed

[✨] Automatic simple 404 responses for file requests #6302

intellix opened this issue May 12, 2024 · 9 comments · Fixed by #6341
Labels
COMMUNITY: good first issue Good for newcomers COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it COMP: performance This issue is related to a performance problem or bug P1: nice to have / fix Not an urgent enhancement or bug (has workarounds) TYPE: enhancement New feature or request

Comments

@intellix
Copy link
Contributor

Is your feature request related to a problem?

I'm not sure why, but every now and again I'll see lots of 404 requests from the service worker for JS files from a previous deployment. I'm guessing the list of files are cached and re-requested despite no longer existing and then they're probably cleaned up by the service-worker as no longer existing.

We're using a custom 404.tsx page which is generated at build time, includes the layout and is around 300kb. When we released a new version I saw around 100x requests for JS that no longer existed and that basically means the browser downloaded 30mb needlessly, slowing down the loading of the page dramatically in poor network conditions.

Describe the solution you'd like

For static file requests, you don't need a pretty 404 page and a smaller response could be delivered. I'm not sure if there's something that can be fixed in the service-worker to not make all of these requests for old JS files that no longer exist.

Describe alternatives you've considered

We're currently doing this inside our entry.cloud.run.tsx to manually fix it:

server.on('request', (req, res) => {
  staticFile(req, res, () => {
    router(req, res, () => {
      // Return a cheaper 404 for static files
      if (req.method === 'GET' && /\.(js|css|jpg|jpeg|png|webp|avif|gif|svg)$/.test(req.url ?? '')) {
        res.writeHead(404, { 'Content-Type': 'text/html; charset=utf-8' });
        res.end('Not Found');
        return;
      }

      notFound(req, res, () => {});
    });
  });
});

Additional context

We're using qwik insights (need to remove until it works for us) and we're currently using an awful prefetchStrategy as it fixes the delays for us when switching pages. We noticed a considerable improve to the user experience when just loading everything and I had no choice but to merge it, even though it's painful. I'm not sure if this is the reason for loading JS that no longer exists but I doubt it:

prefetchStrategy: {
  implementation: {
    linkInsert: 'html-append',
    linkRel: 'prefetch',
    workerFetchInsert: 'no-link-support',
  },
  symbolsToPrefetch: ({ manifest }) => {
    const buildBase = extractBase(opts);
    return Object.keys(manifest.bundles).map((bundleFileName) => ({ url: `${buildBase}/${bundleFileName}`, imports: [] }));
  },
},
@intellix intellix added STATUS-1: needs triage New issue which needs to be triaged TYPE: enhancement New feature or request labels May 12, 2024
@PatrickJS
Copy link
Member

where are you deploying qwik to?

@PatrickJS
Copy link
Member

PatrickJS commented May 12, 2024

looks like google cloud run if you deploy all built files in /build into the same bucket location behind a cdn then this will also fix the problem. in the ssr.entry you can set base to that bucket url

so

base: "https://my-cdn.com/my-path-to-bucket/build/"

@intellix
Copy link
Contributor Author

intellix commented May 12, 2024

not quite sure I'm understanding you :) Everything works as expected, files do exist and are pulled correctly. Our problem (and probably everyone's problem) is that if you make a request for a non-existing JS file (maybe due to caching) that it delivers the 300kb custom JS file instead of a few bytes:

The ethos of qwik is that everything is fast and optimised out of the box and developers don't accidentally fall into performance pitfalls, so delivering your custom 300kb 404 page and causing 30mb of wasteful requests is something that developers have to understand and manually fix.

@PatrickJS
Copy link
Member

ok the solution was setting cloudflare (that is in front of cloud-run) anything with /build to be cache forever

@wmertens
Copy link
Member

@PatrickJS although it wouldn't hurt to have the static file serving for the assets base serve simple 404s

@wmertens wmertens added COMMUNITY: good first issue Good for newcomers COMP: performance This issue is related to a performance problem or bug COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it P1: nice to have / fix Not an urgent enhancement or bug (has workarounds) and removed STATUS-1: needs triage New issue which needs to be triaged labels May 13, 2024
@PatrickJS
Copy link
Member

yup I agree

@PatrickJS PatrickJS assigned PatrickJS and unassigned PatrickJS May 13, 2024
@PatrickJS
Copy link
Member

I have a draft PR here if anyone wants to take over the PR.

Basically remake it and check that everything works. Possible add a test
#6307

@JerryWu1234
Copy link
Contributor

let me try?

okikio added a commit to okikio/qwik that referenced this issue May 20, 2024
Signed-off-by: Okiki <hey@okikio.dev>
@PatrickJS
Copy link
Member

@JerryWu1234 we have a PR for this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMMUNITY: good first issue Good for newcomers COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it COMP: performance This issue is related to a performance problem or bug P1: nice to have / fix Not an urgent enhancement or bug (has workarounds) TYPE: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants