-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: make $app/navigation more resilient to bundler reordering #9808
Conversation
🦋 Changeset detectedLatest commit: 6098df2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -1354,6 +1354,7 @@ export function create_client(app, target) { | |||
return invalidate(); | |||
}, | |||
|
|||
// TODO: Why is only this camel cased? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular good reason, as far as I can tell, since some other things from here that are exposed in packages/kit/src/runtime/app/navigation.js
are snake case in this file and then camel case in that file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added in #6493. I assume it was just a tiny slip-up. I'd be in favor of switching to invalidate_all
.
This does feel like a bit it's addressing the symptoms rather than the cause — I thought we'd already gone to some lengths to ensure that the client is initialised before any user code is imported. Would love to see if we can figure out how that's breaking, though if that turns out to be a massive rabbit hole then I'm not strongly opposed to this approach |
It works well with the default settings, no symptoms or cause there, but once you start overriding it with manualChunks or experimentalMinChunkSize, it breaks because the navigation module ends up in the same chunk as the app entry's dependencies so the exports get evaluated before client is initialized. I tried forcing navigation into its own chunk but even that seems to mess up the order somehow and it's small enough to not warrant its own chunk. The client hooks are one of app's dependencies, so importing |
Ah, fair enough — guess there's really not much else we can do in that case. Merging! Thanks |
fixes #7257
Testing it would require creating a new project I think, and even then it likely wouldn't be deterministic so no tests for this one.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.