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

Lighthouse does not detect my use of modulepreload (recommends using preload) #8945

Closed
philipwalton opened this issue May 13, 2019 · 7 comments

Comments

@philipwalton
Copy link
Member

Provide the steps to reproduce

  1. I ran Lighthouse on my site and got the following opportunity suggestion:

Screen Shot 2019-05-13 at 12 14 20 PM

Note: I'm using <link rel="modulepreload"> instead of <link rel="preload">.

What is the current behavior?

I'm getting a warning for something that I shouldn't be getting a warning for. (Also, I'm not sure if the savings calculated is accurate since I am using modulepreload).

What is the expected behavior?

For scripts loaded via <script type="module"> the recommendation should be to use modulepreload (or perhaps recommend using one or the other).

Environment Information

  • Affected Channels: CLI, DevTools
  • Lighthouse version: 5.0
  • Node.js version: v10.15.3
  • Operating System: macOS 10.14.4

Related issues

@patrickhulce
Copy link
Collaborator

Thanks Phil!

Interesting I can't repro locally. Oh on your staging, nevermind I can repro but I'll leave your fireworks screenshot in here for a congrats :)

image

On to the issue

I only see modulepreload on your main module and the network waterfall seems to suggest your analytics module is indeed later in the process.

image

So Lighthouse appears to be correct in thinking we should load it earlier though why it gets singled out among the other modules isn't super obvious.

What Should LH Do

Seems like we should probably recommend to modulepreload the entire dependency graph or just the root module when it's applicable. I'd lean more towards the entire dependency graph. While the spec technically supports preloading of the entire graph based on just the modulepreload of the first module, AFAICT that still involves the browser fetching each module and learning about its dependencies in a waterfall fashion (please correct me if I'm wrong @philipwalton , my knowledge is weak at best here).

As for text, I agree that we should say modulepreload when we discuss modules but that feels a tad more like a docs distinction. Maybe we just tweak the text to say Consider using <link rel=preload/modulepreload> to ... and leave the rest to the docs?

Side Notes

I'm not super familiar with rel=modulepreload so if other readers are like me this article helped :)

tl;dr - differences in crossorigin attribute behavior mean they couldn't just make as=module work for rel=preload so they made this thing.

@philipwalton
Copy link
Member Author

Interesting I can't repro locally.

Yeah, I just published the new version (using modulepreload). You probably tested it after I'd published to my staging server but but for I published to my primary domain.

Now it looks like this :)

Screen Shot 2019-05-13 at 12 54 03 PM

I only see modulepreload on your main module and the network waterfall seems to suggest your analytics module is indeed later in the process.

That's correct. AFAIK modulepreload only preloads top-level modules, but my analytics module is dynamically imported, which is by design since it's not critical to the user and I don't want to block loading the main logic on analytics.

What Should LH Do

Not 100% sure, but I wouldn't expect to be penalized for not preloading modules I'm dynamically importing.

(Also, FWIW, I do precache all those modules in my SW, so when they are dynamically imported they they should already be in the cache -- depending on when the dynamic import actually happens).

@patrickhulce
Copy link
Collaborator

Not 100% sure, but I wouldn't expect to be penalized for not preloading modules I'm dynamically importing.

Hm interesting yeah I wonder if there's a way to tell without parsing the source whether a module was asynchronously imported. My guess is not right now because the priority Chrome issues your chunk-*.mjs and analytics-*.mjs is both High.

AFAIK modulepreload only preloads top-level modules

Interesting, perhaps my terminology is off then. I was just going off the best practice in that article which was to declare all the deps too.

@philipwalton
Copy link
Member Author

Hmmm, actually I think that article is correct and I was wrong. modulepreload (per the spec) does not require UAs to preload dependencies, and in my testing since filing this issue, it seems that Chrome does not preload dependencies. (I'm asking around to find out why.)

Given that, it's probably still good to recommend using modulepreload on all static (top-level) module dependencies, but I'm not sure if there's a good way to determine that a module is not a top-level, static dependency.

I don't know of a good way via web APIs, but maybe the devtools protocol exposes this information somehow?


Related, I wonder if it makes sense for Lighthouse to warn if module scripts are preloaded with regular <link rel=preload>, as it sounds like doing that does not put them in the module graph.

Again, I can ask around to see what the best practices are here.

@patrickhulce
Copy link
Collaborator

That'd be awesome @philipwalton! We'll hold off making any major changes til we learn more :)

@janosh
Copy link

janosh commented Jan 2, 2021

Interesting discussion here! Any new developments over the last year-and-a-half?

I just ran into the same issue where LH (likely falsely) claims I could save 3,000+ ms on a script that's module-preloaded by adding rel="preload". The url is https://sbsev.netlify.app/blog.

Is this false claim related to the redirect warning in the LH results attached below? LH does not make the same claim when rerunning it on the redirect URL https://sbsev.netlify.app/blog/.

LH

network

@brendankenny
Copy link
Member

preload audits have been disabled in anticipation of some large changes on the Chrome side and some pending changes in Lighthouse. See #11960 for details.

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

No branches or pull requests

4 participants