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

fix(preload): allow ignoring dep errors #18046

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

danielroe
Copy link
Contributor

Description

resolves #18042

This allows individual failures of dep preloads without a chain reaction (which Promise.all forces). By default the behaviour is the same, but if frameworks or users call e.preventDefault on the chunk errors it is still possible for the preload/import to succeed, which is I think valid in the case of CSS failures.

Copy link

stackblitz bot commented Sep 6, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for the issue and PR @danielroe!
We can merge this and later backport it so we release it in v5 too. It seems ok for a patch to me.

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Sep 7, 2024
@@ -93,7 +94,7 @@ function preload(
// in that case fallback to getAttribute
const cspNonce = cspNonceMeta?.nonce || cspNonceMeta?.getAttribute('nonce')

promise = Promise.all(
promise = Promise.allSettled(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that Promise.allSettled is also supported by our default modern browser target.
https://caniuse.com/mdn-javascript_builtins_promise_allsettled

@sapphi-red sapphi-red changed the title fix(vite): allow ignoring dep errors fix(preload): allow ignoring dep errors Sep 9, 2024
@patak-dev patak-dev merged commit 3fb2889 into vitejs:main Sep 9, 2024
13 checks passed
@danielroe danielroe deleted the fix/allow-ignoring-dep-errors branch September 9, 2024 15:23
patak-dev added a commit that referenced this pull request Sep 11, 2024
@patak-dev
Copy link
Member

@danielroe [email protected] has been released backporting this PR

@johndunderhill
Copy link

@patak-dev @danielroe I think this broke dynamic preload error handling:

https://vitejs.dev/guide/build.html#load-error-handling

Since Vite 5.4.4, I am no longer receiving the vite:preloadError event. Something seems to be crashing before the event can be emitted:

image

My preload testing is done as follows:

  1. Build the site for production (with chunking and hashes).
  2. Preview the site.
  3. Browse to the site, but don't touch anything.
  4. Stop the preview server.
  5. Delete the production build.
  6. Make a change.
  7. Rebuild the site.
  8. Verify the hashes of some of the chunks are different.
  9. Restart the preview server.
  10. Go and hover over some of the links (triggers a preload).

Environment is Solidjs with the Solid Router, but I don't think this matters much here. This never fires:

window.addEventListener('vite:preloadError', (event) => {
	console.log('Global vite:preloadError event:', event);
});

John

@johndunderhill
Copy link

@patak-dev @danielroe I've confirmed this. This took a long time to track down.

In Vite 5.4.3, the type error still occurs, but there is no 'uncaught in Promise' and the Vite event is produced as expected:

image

John

@patak-dev
Copy link
Member

That is strange. The code looks correct. @johndunderhill, would you create an issue with a minimal reproduction so we can track and fix this regression?

@danielroe
Copy link
Contributor Author

Yes, I see the behaviour change. Basically errors called when calling baseModule() no longer trigger vite:preloadError. Will raise a PR to ensure that also causes the same error.

@patak-dev
Copy link
Member

Thanks for the report @johndunderhill, and thanks for quickly fixing this @danielroe
It should be resolved in [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

failed CSS preloads prevent loading dependencies
4 participants