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

chore: add JSON schemas for the manifests generated by Vite #7960

Closed
wants to merge 20 commits into from
Closed

chore: add JSON schemas for the manifests generated by Vite #7960

wants to merge 20 commits into from

Conversation

nhedger
Copy link
Contributor

@nhedger nhedger commented Apr 29, 2022

Description

This PR adds JSON schemas for the JSON manifests (manifest.json and manifest-ssr.json) generated by Vite.

  • manifest.schema.json
  • manifest-ssr.schema.json

Both schemas use the 2020-12 draft of the JSON schema specification.

Additional context

Testing the schemas

pnpm run test-build backend-integration

Todo

  • Allow additional chunk properties (for plugins)

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@ElMassimo
Copy link
Contributor

Looks great, thanks for making this proposal! 😃

It should make it a lot easier for plugin authors, as well as make it easier to ensure the ecosystem can follow changes to the format in the future 💯

@nhedger
Copy link
Contributor Author

nhedger commented Apr 29, 2022

Thanks for your suggestions !

@@ -0,0 +1,65 @@
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://vitejs.dev/schemas/v2.0.0/manifest.json",
Copy link
Contributor Author

@nhedger nhedger Apr 30, 2022

Choose a reason for hiding this comment

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

Ideally, it should be possible to retrieve the JSON schemas at this URL.

I've versioned the URL to allow for future schemas under a similar URL but I am unsure about how we want to version the schemas.

Mirroring Vite versions

This would probably add some maintenance burden if not automated. Not sure this is a good option because the schema would not necessarily change with each new Vite version.

Sync only the major version

This option would consist in keeping only the major version in sync with Vite, and minor and path version would be incremented when actual modifications are made to the schemas.

For example, schema v2.0.0 would currently be valid for Vite v2.9.6 generated manifests.

Choose later

We don't actually have to choose the versioning strategy right now.

Let's hear pros and cons or other ideas.

@@ -0,0 +1,14 @@
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://vitejs.dev/schemas/v2.0.0/ssr-manifest.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nhedger
Copy link
Contributor Author

nhedger commented May 11, 2022

Broke my wrist so I'll get back to this in a few weeks.

@patak-dev
Copy link
Member

Broke my wrist so I'll get back to this in a few weeks.

Oh, no rush... Take care! I hope you get better soon

@nhedger
Copy link
Contributor Author

nhedger commented May 28, 2022

I've rebased this PR to make it work with the upcoming 3.0.0 version. Is is too late to change the manifest and ssr-manifest plugins and add an additional version field to them ? I was thinking about something along the lines of:

- export type Manifest = Record<string, ManifestChunk>
+ export interface Manifest {
+   version: string,
+   chunks: Record<string, ManifestChunk>,
+ }

and

+ export interface SSRManifest {
+   version: string,
+   chunks: Record<string, string[]>,
+ }

@patak-dev patak-dev added this to the 3.0 milestone May 28, 2022
@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label May 28, 2022
@patak-dev
Copy link
Member

I added the PR to be discussed in a future team meeting. Vite 3 could give us an opportunity to do this change. I agree that it could be better. Not only for versions, but we may want to include other information in the future.

@patak-dev
Copy link
Member

@nhedger we discussed with the team and we would like to avoid changing the schema to add a version at this point. So we could document the current format (taking into account #6649, in case that needs to be also reflected)

@patak-dev patak-dev removed this from the 3.0 milestone Jun 20, 2022
@netlify
Copy link

netlify bot commented Jun 29, 2022

Deploy Preview for vite-docs-main canceled.

Name Link
🔨 Latest commit 4b5fdad
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62bc702e4fd4890008a57727

@nhedger
Copy link
Contributor Author

nhedger commented Jun 29, 2022

@patak-dev I believe #6649 does not introduces any changes to the schema. I've removed the versions from the $id strings of both schemas.

Edit: failing tests do not appear to be linked to this PR.

@nhedger nhedger marked this pull request as ready for review June 29, 2022 16:05
@bluwy
Copy link
Member

bluwy commented Jul 1, 2022

@nhedger Since this won't be published to npm, and not served under https://vitejs.dev yet, do you have plans on how the new JSON schemas are used? You mentioned that ideally we'd serve under https://vitejs.dev/schemas/v2.0.0/schema.json for example, is this still needed?

The spec shows that $id doesn't need to be a downloadable URI, so the state of the PR as is should be good too. But with that I'm curious how this file would be used in practice?

@patak-dev
Copy link
Member

We aren't currently publishing docs for each patch (we should though), so if we add the version in the URL, it should only use the minor.

@nhedger
Copy link
Contributor Author

nhedger commented Jul 2, 2022

@nhedger Since this won't be published to npm, and not served under https://vitejs.dev yet, do you have plans on how the new JSON schemas are used? You mentioned that ideally we'd serve under https://vitejs.dev/schemas/v2.0.0/schema.json for example, is this still needed?

The spec shows that $id doesn't need to be a downloadable URI, so the state of the PR as is should be good too. But with that I'm curious how this file would be used in practice?

Good point, I actually forgot about that.

Currently, the only way of accessing the schema would be through GitHub (once merged into main):

https://raw.githubusercontent.com/vitejs/vite/main/packages/vite/src/node/schemas/manifest.json
https://raw.githubusercontent.com/vitejs/vite/main/packages/vite/src/node/schemas/ssr-manifest.json

Assuming that everyone agrees, I may be able to setup a GitHub Action to automate copying the schemas to /docs/public/schemas. This could be triggered everytime a new version of Vite is tagged. The schemas would then be served under the URL specified in the $id field.

@bluwy
Copy link
Member

bluwy commented Jul 2, 2022

If we automate serving them through the public folder, we'd only be able to serve the latest JSON schema at a point of time. So if an old schema points to the URL, and returns a new different schema, would that be a problem?

We could probably solve it by putting the version in the URL, and do a 301 redirect and rewrite the URLs to githubusercontent. But with all these said, I'm be curious to know if the team is fine with this.

@nhedger nhedger closed this by deleting the head repository Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants