This repository was archived by the owner on Sep 30, 2024. It is now read-only.
chore(svelte): Refactor repo loaders#63828
Merged
Merged
Conversation
camdencheek
approved these changes
Jul 16, 2024
| // subpages that if they load the requested revision exists. This | ||
| // relieves subpages from testing whether the revision is valid. | ||
| const { repoName, resolvedRevisionOrError } = await parent() | ||
| const { revision, defaultBranch, resolvedRepository, repoName } = await parent() |
Member
There was a problem hiding this comment.
Oh very nice. It was always annoying to have to worry about resolution errors in child pages.
Contributor
Author
There was a problem hiding this comment.
You didn't have to worry about it because this loader would ensure that resolvedRevision is not an error. But the way this was all set up was a bit convoluted IMO.
| repo: resolvedRepository, | ||
| commitID: commit.oid, | ||
| defaultBranch, | ||
| } satisfies ResolvedRevision, |
| client, | ||
| repoName, | ||
| revspec: revision, | ||
| url: untrack(() => new URL(url)), |
Member
There was a problem hiding this comment.
Mind leaving a comment on why we need untrack here?
Contributor
Author
There was a problem hiding this comment.
I removed untrack. I originally though we needed it since we don't want the loader to rerun when the URL changes, but since the URL is only really read when creating a redirect to a different host this is not necessary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit is in preparation for other work. It refactors how we work with resolved repository and resolved revision information.
The idea was that the top level repo loader should try to resolve the repository information and error if that wasn't possible. Not being able to resolve the revision was accepted, hence this check:
However the way it was implemented meant that we wouldn't pass any resolved repository information to the sub-pages/layouts when the revision couldn't be resolved, which seems wrong.
With these changes, the top level repo loader now provides a
resolvedRepositoryobject (wherecommit/changelistmight be unset) and the(validrev)loader creates theresolvedRevisionobject, just how its sub-pages/layouts expect.And instead of returning error objects from
resolveRepoRevisionand checking them in the loader we throw errors/redirects directly in that function. IMO that makes the whole flow easier to understand.Test plan
Manual testing, CI integration tests