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: use unknown instead of any for additional properties #1466

Merged

Conversation

jmorel
Copy link
Contributor

@jmorel jmorel commented Jun 19, 2024

Status

READY

Description

Fixes #1464

The generated type for objects with additionalProperties: true becomes [key: string]: unknown instead of [key: string]: any for better type safety.

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

> git pull --prune
> git checkout <branch>
> grunt jasmine

@jmorel jmorel marked this pull request as ready for review June 19, 2024 07:21
@ezequiel
Copy link
Contributor

@jmorel
Copy link
Contributor Author

jmorel commented Jun 19, 2024

Yes definitely, I only concentrated on additionalProperties but it makes sense to fix the issue more broadly.

@jmorel jmorel force-pushed the use-unknown-for-additional-properties branch from cd7cf19 to f89c0b7 Compare June 19, 2024 07:36
Copy link
Contributor

@ezequiel ezequiel left a comment

Choose a reason for hiding this comment

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

:shipit:

@melloware melloware added the bug Something isn't working label Jun 19, 2024
@melloware melloware added this to the 6.31.0 milestone Jun 19, 2024
@melloware
Copy link
Collaborator

The original fix was here in 2021: #190 But I agree unknown is safer than any

@melloware melloware merged commit 950b547 into orval-labs:master Jun 19, 2024
2 checks passed
@uykms
Copy link

uykms commented Jul 1, 2024

The Typescript checking of my source is invalid once again. If you want to change from any to unknown, please create a new property in the config file to allow other users to decide whether to apply your change.
Thank you!
cc @jmorel @ezequiel

@ezequiel
Copy link
Contributor

ezequiel commented Jul 1, 2024

The Typescript checking of my source is invalid once again. If you want to change from any to unknown, please create a new property in the config file to allow other users to decide whether to apply your change. Thank you! cc @jmorel @ezequiel

unknown should've arguably always been the default.

I'm personally not a big fan of making this configurable, simply because any is too dangerous as it will mask potential type errors in your code.

To quickly move forward and upgrade Orval, I suggest adding // @ts-expect-error above the offending lines of code, and resolve them later when time permits.

e.g

// @ts-expect-error todo: fix this 
const thing = data.response.thing;

@lessquo
Copy link

lessquo commented Jul 5, 2024

Hi, I've just noticed this change when I tried the latest version. I agree that unknown is much safer than any, however this breaking change is a big pain for my huge codebase. It would be really nice if I can incrementally adopt this change.

In addition to that, is there any way to configure each object type with typescript interface? because I have a lot of 3rd-party dependencies that provide types with interface in the backend, but they all converted into object due to typescript limitation, and then the properties become unknown in the frontend. They are actually not 'unknown' because I can provide types for them. Since editing the auto-generated files is not a good idea, an option to configure them would be ideal.

This is just a small feedback. Thanks for the amazing work

@FranjoMindek
Copy link

FranjoMindek commented Aug 6, 2024

Same here, the change broke any location where we explicitly typed a generic object from the back end.
Instead of using:

const data.someObject as SomeType;

We now need to use:

const data.someObject as unknown as SomeType;

(truthfully back end is the root cause of the problem for our specific project, which should type out all the possibilities rather than generic object, but back end is something I don't have access to, and it showcases that this change can break the codebase for some use cases)

@melloware
Copy link
Collaborator

Yes this issue has gone back and forth between any and unknown with both sides arguing why one is better than another.

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.

Use unknown instead of any for additional properties type
6 participants