-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
refactor(ui): Loading schema dynamically for dataset profile #7558
refactor(ui): Loading schema dynamically for dataset profile #7558
Conversation
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.
nice! two quick questions then let's do this
}); | ||
const isHideSiblingMode = useIsSeparateSiblingsMode(); | ||
// Merge with sibling information as required. | ||
const combinedData = rawData && !isHideSiblingMode ? combineEntityDataWithSiblings(cloneDeep(rawData)) : rawData; |
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.
does the cloneDeep
here just ensure we don't mess with the original rawData
object in this method? is that necessary in the other places we call combineEntityDataWithSiblings
or only here because it's cache-first
?
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.
it's because of cache! apollo hates it when we f with the objects it has in cache!
urn, | ||
}, | ||
skip: entityType !== EntityType.Dataset, | ||
fetchPolicy: 'cache-first', |
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.
is editing schema field documentation, tags, and terms affected here with cache-first
?
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.
i've actually updated to address!
export const useGetEntityWithSchema = () => { | ||
const { urn, entityData, entityType } = useEntityData(); | ||
// Load the dataset schema lazily. | ||
const { data: rawData, loading } = useGetDatasetSchemaQuery({ |
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.
also i think this query got lost in the commit so it's not defined? or is it already defined?
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.
it should be inside dataset.graphql!
remove all ref to schema metadata Adding back test changes Adding schema context fix checkstyler Add new data to ensure test passes
…om/jjoyce0510/datahub-1 into jj-load-schema-dynamically-dataset
…et' into jj-load-schema-dynamically-dataset
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…-project#7558) Co-authored-by: david-leifker <114954101+david-leifker@users.noreply.github.com>
…-project#7558) Co-authored-by: david-leifker <114954101+david-leifker@users.noreply.github.com>
Summary
We've found that large schemas can bottleneck the time to first meaningful render on the Dataset profile page. This PR speeds things up by separating the schema tab load from the main getDataset query (for datasets only).
Features include a new loading indicator on the schema tab while the fetch for the schema loads. We also use the apollo cache to ensure that switching between tabs remains smooth.
Status
Ready for review.
Checklist