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
feat(sanity): memoize initial value resolver #6614
Merged
Merged
Conversation
This file contains 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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
juice49
force-pushed
the
fix/sdx-1321-memoize-initial-value-resolver
branch
from
May 10, 2024 15:50
5661372
to
dbc653a
Compare
No changes to documentation |
Component Testing Report Updated May 16, 2024 3:59 PM (UTC)
|
juice49
force-pushed
the
fix/sdx-1321-memoize-initial-value-resolver
branch
from
May 10, 2024 16:06
dbc653a
to
dde6309
Compare
juice49
force-pushed
the
fix/sdx-1321-memoize-initial-value-resolver
branch
from
May 10, 2024 18:03
dde6309
to
7ae1d01
Compare
juice49
force-pushed
the
fix/sdx-1321-memoize-initial-value-resolver
branch
from
May 10, 2024 21:47
f021f01
to
978d853
Compare
juice49
changed the title
wip: memoize initial value resolver
feat(sanity): memoize initial value resolver
May 10, 2024
juice49
force-pushed
the
fix/sdx-1321-memoize-initial-value-resolver
branch
from
May 10, 2024 21:48
978d853
to
acc1695
Compare
binoy14
reviewed
May 13, 2024
juice49
requested review from
sjelfull and
binoy14
and removed request for
a team
May 14, 2024 15:52
juice49
force-pushed
the
fix/sdx-1321-memoize-initial-value-resolver
branch
from
May 15, 2024 13:14
1c476da
to
ea5d7ce
Compare
binoy14
previously approved these changes
May 15, 2024
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.
Makes sense to me, thanks!
ricokahler
dismissed
binoy14’s stale review
May 15, 2024 21:56
The merge-base changed after approval.
juice49
force-pushed
the
fix/sdx-1321-memoize-initial-value-resolver
branch
from
May 16, 2024 15:49
ea5d7ce
to
423134b
Compare
binoy14
approved these changes
May 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Wherever Studio allows users to create new documents, it runs
getTemplatePermissions
, which subsequently resolves the initial value of every schema type. It's troublesome for schema types that use aninitialValue
function (especially async function that may call an HTTP API), because it causes that function to be called at multiple unexpected times.initialValue
function, aPaneHeaderCreateButton
is rendered.ReferenceInputOptionsProvider
is rendered.We cannot simply remove initial value resolution. Studio must resolve initial values before allowing the user to create a new document in case those values would preclude the user from viewing the document they just created. e.g. if the user is prohibited from viewing documents where the
secret
field istrue
, they should not be given the option to create such documents.In order to minimise unnecessary calls to the
initialValue
function, this branch memoizes theinitialValue
function when it's called fromgetTemplatePermissions
.When a document is created, memoization isn't applied. This ensures newly created documents aren't given stale values. A contrived example of when this would be an issue is if an
initialValue
function returns the current time. This does mean it would be possible for the user to create a document they don't have permission to view if some piece of data controlling this changed since the initialgetTemplatePermissions
call, but this scenario seems unlikely.To see this for yourself, try tracking how many times an
initialValue
function is called when loading Studio with the document pane visible.Before change: 42 calls
After change: 3 calls (once for the global document creation UI, once for the document list document creation UI, and once to populate the new document)
What to review
Does this approach make sense? Are there any scenarios I've missed (such as various initial value template configurations)?
Testing
Added tests in
packages/sanity/src/core/templates/__tests__/resolve.test.ts
.Notes for release
Adds memoization to minimise initial value function calls.