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

fix(core): add missing properties in subschemas to fix TS error #1115

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

o-alexandrov
Copy link
Contributor

@o-alexandrov o-alexandrov commented Dec 19, 2023

closes #935

Status

READY

Description

Fixes TypeScript error when accessing a property from a union of objects with different schema.
Please refer to the provided example error in #935

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

Steps to Test or Reproduce

Run locally to observe the generated types changed (now have missing properties declared as missingProperty?: never in:
- samples/react-query/basic/src/api/model/dog.ts
- samples/react-query/basic/src/api/model/pet.ts

Screenshots of the changes Screenshot 2023-12-19 at 9 20 21 AM Screenshot 2023-12-19 at 9 19 59 AM

@o-alexandrov
Copy link
Contributor Author

@melloware could you please guide on what's missing here to get it merged?

@melloware
Copy link
Collaborator

The build is failing with "Code style issues found in 25 files. Forgot to run Prettier?"

@melloware melloware added this to the 6.24.0 milestone Dec 19, 2023
Signed-off-by: Olzhas Alexandrov <[email protected]>
@melloware melloware added the bug Something isn't working label Dec 19, 2023
@melloware melloware requested a review from anymaniax December 19, 2023 15:23
@o-alexandrov
Copy link
Contributor Author

@melloware in terms of prettier, I see there are multiple versions installed (see screenshot)
imho, if I make formatting changes in this PR, I'll add unrelated changes, making it harder for you to review. (you can confirm it by running prettier locally)
In other words, I think the formatting is messed up before this PR.

To fix formatting, we should consider to either:

  • remove prettier and other devDependencies from all samples' packages package.json files except the repo's root.
  • add samples/* to workspaces in repo's root package.json

Or run formatting in a separate PR.

Screenshot of prettier versions within the repo Screenshot 2023-12-19 at 9 26 03 AM

@melloware
Copy link
Collaborator

@Maxim-Mazurok weren't you proposing something like what @o-alexandrov is asking above?

@Maxim-Mazurok
Copy link
Contributor

I'm pretty sure formatting was fine in the whole project on my PRs. And I may have added prettier checks into CI pipeline to keep it that way. Maybe someone pushed directly into master unformatted code without going through a PR? That would explain unformatted code.

Multiple prettier versions shouldn't be an issue, there's a command to do the formatting in the root project I believe.

Hope this helps. Regarding review I might take a look sometime, but can't promise.

@Maxim-Mazurok
Copy link
Contributor

And IMO - just format it. If it's a really big PR then do another formatting PR, then after it's merged do this one. We can't just merge with failing CI.

@o-alexandrov
Copy link
Contributor Author

o-alexandrov commented Dec 19, 2023

I didn't claim multiple versions of prettier is an issue in the formatting itself.

  • it's a duplication for no reason with performance degradations and actual format differences

For your convenience, I'll add a separate PR to fix formatting and then rebase this PR, so this PR contains only related changes to the title

EDIT: formatting regression is addressed in #1117

@melloware melloware merged commit b889c88 into orval-labs:master Dec 19, 2023
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid generated typings for a case w/ multiple objects w/ different schemas
3 participants