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

Feat/studio manifests cont #7403

Open
wants to merge 51 commits into
base: next
Choose a base branch
from
Open

Feat/studio manifests cont #7403

wants to merge 51 commits into from

Conversation

snorrees
Copy link
Contributor

@snorrees snorrees commented Aug 21, 2024

Studio manifest for Create

The Create project needs access to workspaces and schema define in the Studio.

Create will use the schema to allow users to automatically map their documents into a Studio document type.
Create needs workspace information to deep-link into studios.

This PR introduces "manifest extract", which makes workspace and schema details HTTP accessible for sanity deploy'ed studios.

Timeline

Create is fine with using a tagged release of studio until this PR gets merged and released.

PR includes

  • Everything and the kitchen-sink manifest format tailored for Create:
    • For every workspace, we include most everything that is serializable on the schema; options, unknown properties, the works.
    • Validation is serialized to a best-effort format, including any serializable data from the RuleSpec.
    • This allows Create to specify a schema-driven API for configuring Create-Studio integration, as needed, without needing changes to core.
  • New command sanity manifest extract.
    • No external users should rely on the output files. They are for internal use only.
  • Runs manifest extract during sanity deploy commands
    • This becomes default behavior, and there is no opt out.
    • When manifest extract fails, we show a soft warning; it should never result in a failed deploy.
    • Note, we do NOT run manifest extract for sanity build
    • 2 minute timeout

The following files are produced

  • create-manifest.json
  • <schema-hash>.create-schema.json

By default these gets written to /dist/static, and will therefor be served from <studio-url>/static/<filename> when using sanity deploy.

Custom deploys will be expected to serve the files under <studio-url>/static/ as well.
For Next.js, this can be done by using sanity manifest extract --path /public/static && next build, and putting the generated files on .gitignore.

History

@juice49 graciously provided most of the implementation; I have adjusted it further as the requirements has changed.

Eror output

If the extract fails, you get this error:

❌️ Extracting manifest
Unable to extract manifest. Certain features like Sanity Create will not work with this studio.
To disable manifest extraction set SANITY_CLI_EXTRACT_MANIFEST_ENABLED=false

Manifest extract will not stop deployment.

Create - Studio integration

  • Upgrade your studio to the new version
  • Deploy
  • The studio (with schema) can now be selected in Create

For a broader discussion internally, refer to this SCQA

What to review

  • Are we ok with the proposed command?
  • Is it ok that we dont include a --format argument at this time?
  • Think about failure cases with sanity deploy. Are there ways in which manifest extract could prevent sanity deploy from working?
  • Do we need to worry about hypothetical timeouts with the extract task?

Headsup: some of the files are kinda big; remember to expand them in the diff when reviewing.

Testing

Consider the jest tests: are they exhaustive enough?

Manual testing

In a project you can deploy using sanity deploy:

  • npm i sanity@manifests
  • npm run deploy (or npm run build && npm run start to run on localhost)
  • Visit <studioHost>/static/create-manifest.json
  • Visit a workspace schema (<studioHost>/static/<schema-path-found-in-workspace> on the form <schema-hash>.create-schema.json)

Check the SCQA doc for how to test with Create

Testing commands:

  • sanity should include manifest command group
  • sanity manifest should list sub commands (currently only extract)
  • sanity manifest extract --help should show help for the new command
  • sanity manifest extract in a project should extract the manifest (or fail gracefully)

Regression testing

  • sanity schema extract should work as before
    (this branch should not make any changes to the codepath, this is just here to sanity check that reverted changes didnt make it back in)

Known issues

  • test-studio has "too complex" schema; manifest extract as it does not work for these kinds of studios.
    • manifest extract has the same limitations as graphql deploy; the schema has to be parsable in the mock-browser env we are using
  • the extracted manifest does not extract structure paths (possibly needed to create deep-links for certain configurations)
  • the extracted manifest not extract auth provider (possibly needed to correctly show auth screen for studio login inside Create)

Notes for release

Introduces a new command, so this needs consideration.
TODO: Figure out what needs to go into release docs. Should probably link to a Create - Studio howto document?

Copy link

vercel bot commented Aug 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2024 0:07am
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2024 0:07am
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2024 0:07am
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2024 0:07am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2024 0:07am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Sep 19, 2024 0:07am

juice49
juice49 previously approved these changes Sep 18, 2024
Copy link
Contributor

@juice49 juice49 left a comment

Choose a reason for hiding this comment

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

This looks great, Snorre! Thank you for picking up my work. I've left a couple of small comments, and also bumped this with the Studio team to see if anybody else is able to take a look.

name: 'manifest',
signature: '[COMMAND]',
isGroupRoot: true,
description: 'Interacts with the studio configuration.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this description correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I was a bit unsure about what to put here. Manifest sort of looks at the whole studio config and extracts part of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For context schemaGroup reads: Interacts with Sanity Studio schema configurations

}

try {
await extractManifest(args, context)
Copy link
Contributor

@juice49 juice49 Sep 18, 2024

Choose a reason for hiding this comment

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

Could it be a good idea to add a timeout to this operation, at least for initial iterations before we understand how it performs for various schemas in the wild (and in different environments)? We could set a fairly lengthy value (1 or 2 minutes, perhaps) that would at least ensure we don't block folks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, was considering it but was a lazy boi 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a 2 minute timeout.

Also added a line about which env var to set if you run into an extract error - probably useful if folks run into the timeout. Adding 5 seconds to a built is one thing, adding 2 minutes quite another 😓

@ryanbonial
Copy link
Contributor

This looks good to me, but we should have someone with more knowledge about this area also take a look. @ricokahler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants