Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

[Rollup] Remove client.js from Link header #449

Closed
lukeed opened this issue Sep 24, 2018 · 7 comments · Fixed by #1269
Closed

[Rollup] Remove client.js from Link header #449

lukeed opened this issue Sep 24, 2018 · 7 comments · Fixed by #1269

Comments

@lukeed
Copy link
Member

lukeed commented Sep 24, 2018

On the Rollup version only, the bundle_info.assets.main JS bundle should not be included in the Link response header.

While that would technically be the correct action to take (and continues to be with the Webpack template), it results in a double-download of the main file on any application produced with Rollup.

The reason is because the main JS file is passed thru Shimport in every scenario, and so the document cannot infer that the preloaded asset can be reused. This can be verified by removing the Link header and instead using link rel=preload as=script tags instead. In this case, there's a JS console warning about the client.*.js not being used within a few seconds, despite the fact that was... it was just that a second request was made instead.

Contrast this with the Webpack template, which does have a script src=* tag directly on the page. Here, the browser can correctly infer that the preloaded asset goes <here>.

The easiest solution is to remove the client.*.js file from the preloaded_chunks array, and do a "raw" request or the file thru Shimport. Since the page is SSR'd & isn't interactive until all chunks have finished loading anyway (thru Shimport), this wouldn't delay the page's timeline by much.

^ I have a local fix for this & can push it up if desired.

The other option is to do the browser-compat checks in the server middleware instead of waiting to begin on DOMContentLoaded. You'd sniff the UA string that comes in & serve the appropriate bundle-set immediately.

This is something I've seen @kristoferbaxter do well with his preact-hn and is something I've considered working into sirv – or perhaps another module that builds from it.

And finally, here's a network timeline for the base Rollup template, running on a simulated 3G.

screen shot 2018-09-24 at 2 44 07 pm

@arxpoetica
Copy link
Member

@lukeed what's your local fix? mind sharing?

@mrkishi
Copy link
Member

mrkishi commented Oct 2, 2018

This seems to happen regardless of whether shimport is being used or not.

I don't know what the best course of action is, but the browser (Chrome, at least) won't match the preloaded scripts with scripts of type="module". When using shimport, it won't match them because they should be fetch instead of script.

More info in this article, with an even more experimental feature modulepreload. I guess it would be safe to swap the current preload for a modulepreload, since the latter will only work on browsers that also do not load shimport.

Update: Actually, as of #459, adding a crossorigin="use-credentials" to the link header will cause a match. Could someone test it with Safari? The other browsers don't support preload yet (except for Edge, but not as a header), so if it also matches, simply adding crossorigin should be enough for now.

@lukeed
Copy link
Member Author

lukeed commented Oct 2, 2018

@mrkishi Very interesting, thanks! I didn't know about this 🙌

@arxpoetica Sure, I didn't PR because it's not really a good solution IMO, but it does work in that it prevents the double download.

In the latest version of Sapper, these lines should become:

        // TODO detect other stuff we can preload? images, CSS, fonts?
        let main_chunk = build_info.bundler === 'rollup' ? [] : build_info.assets.main;
        let preloaded_chunks = Array.isArray(main_chunk) ? main_chunk : [main_chunk];
        if (!error) {
            page.parts.forEach(part => {
                if (!part)
                    return;
                // using concat because it could be a string or an array. thanks webpack!
                preloaded_chunks = preloaded_chunks.concat(build_info.assets[part.name]);
            });
        }
        const link = preloaded_chunks
            .filter(file => file && !file.match(/\.map$/))
            .map(file => `<${req.baseUrl}/client/${file}>;rel="preload";as="script"`)
            .join(', ');
        if (link) res.setHeader('Link', link);

@lukeed
Copy link
Member Author

lukeed commented Oct 2, 2018

Both of @mrkishi's suggestions seem to work as well, including on Safari

@arxpoetica
Copy link
Member

arxpoetica commented Oct 2, 2018

Anyone want to do a PR of @mrkishi's fix? I'm not following at all :(

(I'm also worried this is the thing that's breaking my build in production, though I don't have evidence...)

@benmccann
Copy link
Member

@lukeed is this still an issue? I sent #1269 which preloads all assets including the client.js and I'm not seeing client.js requested twice in the network tab

@lukeed
Copy link
Member Author

lukeed commented Jul 16, 2020

Not sure tbh; nearly two years ago 🙈

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants