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

Update to ajv 8 and react-json-schema-form 5 #13924

Merged
merged 13 commits into from
Feb 8, 2023

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Feb 4, 2023

References

Code changes

  • add version 5 of @rjsf/core, @rjsf/utils and @rjsf/validator-ajv8
  • update to ajv8
  • add explicit version property to plugin schema
  • remove submit button with default ui:options rather than CSS (the .rjsf class is gone).
  • applies (and removes) TODOs about replacing home-grown types with RJSF ones

User-facing changes

  • forms return somewhat more useful error messages
master #13924

Backwards-incompatible changes

  • the ajv instances wasn't strictly exposed, so this probably doesn't change much
  • the validator prop on FormComponent is required
  • the props of custom fields, etc. will change

Feedback Sought

🛎️ @brichet @marthacryan @krassowski

Future Work

A follow-on PR will refactor the dependencies on ajv 8 and @rjsf/validator-ajv8 in a single @jupyterlab/validator-ajv8.

This will allow for cleanly replacing the runtime JSON Schema-related dependencies served to the browser by disabling the default provider.

@jupyterlab/builder could be changed (in another PR) to use a correct, but less-performant validator that isn't flagged for unsafe-eval, or somehow pre-compiles all the schema with ajv-cli.

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@bollwyvl bollwyvl marked this pull request as draft February 4, 2023 23:18
@bollwyvl bollwyvl added the dependencies Pull requests that update a dependency file label Feb 4, 2023
@bollwyvl bollwyvl marked this pull request as ready for review February 5, 2023 16:26
@bollwyvl bollwyvl mentioned this pull request Feb 5, 2023
13 tasks
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this @bollwyvl

I have one question.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 6, 2023

Thanks for the speedy review.

The follow-on work is proving quite tricky: moving the form validator out to a plugin/package in #13927 is no problem (aside from the CI complaints about installing not-on-npm packages), but trying to generalize the way settings (and other future consumers) use a number of ajv's features (like merging default values in-place) is challenging to think about without having a specific implementation in mind.

Of the recognized JS implementations, many appear unmaintained. Of the remaining, few have these capabilities, or the model of a "validator factory."

On the general topic of other implementations: all of this smells somewhat like security theater, as oh noes! unsafe-eval! in full JupyterLab is kind of... silly. I'd much rather spend time getting a nice, reusable lumino form for extension authors working for 4, but the architectural stuff seems like it is important to at least consider.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 6, 2023

Ubuntu's mirrors are having A Day, so i wouldn't sweat the snapshot tests failing at the moment. Not sure what the right approach is to get access to more mirrors.

* Switch to VEGALITE5_MIME_TYPE

* Update test to Vega-Lite 5

* Update documentWidgetFactoryOptions for Vega-Lite5

* Update vega5 extension description
Bumps [http-cache-semantics](https://github.com/kornelski/http-cache-semantics) from 4.1.0 to 4.1.1.
- [Release notes](https://github.com/kornelski/http-cache-semantics/releases)
- [Commits](kornelski/http-cache-semantics@v4.1.0...v4.1.1)

---
updated-dependencies:
- dependency-name: http-cache-semantics
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Bumps [tj-actions/changed-files](https://github.com/tj-actions/changed-files) from 35.4.4 to 35.5.0.
- [Release notes](https://github.com/tj-actions/changed-files/releases)
- [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md)
- [Commits](tj-actions/changed-files@v35.4.4...v35.5.0)

---
updated-dependencies:
- dependency-name: tj-actions/changed-files
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
jtpio and others added 3 commits February 6, 2023 21:53
* Bump ruff from 0.0.238 to 0.0.241

Bumps [ruff](https://github.com/charliermarsh/ruff) from 0.0.238 to 0.0.241.
- [Release notes](https://github.com/charliermarsh/ruff/releases)
- [Changelog](https://github.com/charliermarsh/ruff/blob/main/BREAKING_CHANGES.md)
- [Commits](astral-sh/ruff@v0.0.238...v0.0.241)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Deal with new errors

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
@brichet
Copy link
Contributor

brichet commented Feb 7, 2023

Thanks @bollwyvl, that looks good to me.

Do you think we could do without the IMetadataSchema described in settingregistry ? It seems that its properties already exist in RJSFSchema.
It can also be done in a follow up PR, as I don't know the consequences of changing the properties property.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 7, 2023

Yeah, on this PR was looking to keep stuff similar-ish when i could.

But it indeed needs a further specialization of RJSFSchema to capture having properties, and or any special sauce.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 7, 2023

settingregistry

Actually, to that point: I think we don't want any RJSF (or any R for that matter) imports (even just type ones) in places we don't absolutely need them. By having a custom, documented type in the plumbing package, we are a little more able to keep stuff consistent if rjsf6 or whatever comes along and changes things during the lab 4.x line.

This is part of my motivation for breaking some more of these things up, and providing more Lab-owned APIs than relying on R/JSF: as an extension author, relying on

import {SchemaForm} from '@jupyterlab/ui-components'
...
activate: (app) => {
  //...
  const form = new SchemaForm({schema, instance});
  form.submitted.connect(() => app.commands.execute('some-command', form.instance))
}

...will require caring much less over time about the underlying packages, and getting to UI+validation quickly.

If an author wades into heavy customization, that's up to them! Indeed, I'd really like to be able to wrap a lumino Widget into the registry (i can taste the datagrid as an ArrayField) but that's real far down the line, and I'm unlikely to get to it in the 4.0.0 window.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @bollwyvl

As you mentioned, let's move on with the package upgrade with this PR. And leave further enhancement for follow up tasks.

@fcollonval fcollonval merged commit b7c2414 into jupyterlab:master Feb 8, 2023
@bollwyvl bollwyvl deleted the gh-13746-update-json-schema branch February 8, 2023 17:26
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants