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

Much more robust auto-completion suggestions for conditional types (even when current data is invalid) #138

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

thomasjahoda
Copy link
Contributor

@thomasjahoda thomasjahoda commented Aug 16, 2024

I continued the work on properly supporting conditional types. Conditional types were first supported via #133, quite recently.
A lot of cases were not working at all, specifically whenever the data was not valid for the current schema and whenever a new property was being added in a dynamic schema, where the parent does not have enough info for the types.

I added some tests for the scenarios I needed to support. And after verifying the old tests are working, I added additionalProperties: false to some existing test-data schemas, as that complicates matters.

@imolorhe I didn't see your work in #135 (branch conditional-prop-values), only the commits already merged into master due to #133. I chose the same approach as you to workaround the limitations of json-schema-library: to get the list of all properties in the current object, I just ask json-schema-library to resolve the JSON pointers for all possible direct properties I can find.

Copy link

changeset-bot bot commented Aug 16, 2024

⚠️ No Changeset found

Latest commit: 4055b35

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Aug 16, 2024

Deploy Preview for codemirror-json-schema ready!

Name Link
🔨 Latest commit f5332d2
🔍 Latest deploy log https://app.netlify.com/sites/codemirror-json-schema/deploys/677c2a794d05eb00087c866f
😎 Deploy Preview https://deploy-preview-138--codemirror-json-schema.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@acao
Copy link
Collaborator

acao commented Sep 6, 2024

@thomasjahoda thank you for this! I'm currently looking into the weird vitest failure. can you add a changset as well? pnpm changeset add etc

@@ -161,7 +161,7 @@ const getSchema = async (val: string) => {
const schemaSelect = document.getElementById("schema-selection");
const schemaValue = localStorage.getItem("selectedSchema")!;

const setFileName = (value) => {
const setFileName = (value: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not a huge deal because it's just the demo, but wouldn't this be string?

@@ -7,4 +7,10 @@ export default defineConfig({
ignoreConfigErrors: true,
}),
],
test: {
maxConcurrency: 10,
// configuration to be able to view console.log messages while debugging
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we may need to disable or toggle something here in Github Actions

* removes required properties and allows additional properties everywhere
* @param schema
*/
function makeSchemaLax(schema: any): any {
Copy link
Collaborator

Choose a reason for hiding this comment

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

😎

vite.config.ts Outdated Show resolved Hide resolved

// TODO also resolve patternProperties of allOf, anyOf, oneOf
return {
...omit(subSchema, ["allOf", "anyOf", "oneOf"]),
Copy link
Collaborator

@acao acao Sep 7, 2024

Choose a reason for hiding this comment

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

there is an easier way to do this that doesn't require an additional dependency. radash looks cool but I don't think we need it right now, what do you think @imolorhe ?

an ecmascript native approach would be something more like this:

const {
 allOf, anyOf, oneOf, 
 ...simpleSchema
} = subSchema

return {
   ...simpleSchema,
  properties: effectiveProperties,
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah we shouldn't be adding that extra dependency.

@acao
Copy link
Collaborator

acao commented Sep 9, 2024

@thomasjahoda i merged your other PR first so this needs a rebase. just let me know when you are able to remove radash for now and we're good to go!

@acao
Copy link
Collaborator

acao commented Nov 18, 2024

@thomasjahoda are you interested in continuing this PR?

@acao
Copy link
Collaborator

acao commented Dec 17, 2024

heads up @imolorhe I plan on re-creating this PR and making the one change we need, if we don't hear back from @thomasjahoda soon

@thomasjahoda
Copy link
Contributor Author

@acao Sorry that I did not react to my inbox at all. Sadly, I don't think that I should spend more time on this in the near future and I would be very glad if someone else continues this. Please feel free to use/change my code/PR however you like.
And thanks for maintaining this great extension!

@acao acao merged commit aa27ad7 into jsonnext:main Jan 6, 2025
4 checks passed
@github-actions github-actions bot mentioned this pull request Jan 6, 2025
acao pushed a commit that referenced this pull request Jan 6, 2025
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## [email protected]

### Minor Changes

- [#138](#138)
[`aa27ad7`](aa27ad7)
Thanks [@thomasjahoda](https://github.com/thomasjahoda)! - More robust
conditional types support (thanks @thomasjahoda!)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

3 participants