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

Remove openapi3-ts exports #150

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Conversation

dmarcato
Copy link
Contributor

These exports seem wrong, you can just import directly from the openapi3-ts dependency without going through this library.

On top of that, they are actually the source of an error when using this project as a dependency in a yarn project:

Module not found: Can't resolve 'openapi3-ts/oas30' in '[...]/node_modules/@asteasolutions/zod-to-openapi/dist'

I believe it's safe to remove them, was there any specific reason why they were added?

These exports seem wrong, you can just import directly from the `openapi3-ts` dependency without going through this library.

On top of that, they are actually the source of an error when using this project as a dependency in a `yarn` project:
```
Module not found: Can't resolve 'openapi3-ts/oas30' in '[...]/node_modules/@asteasolutions/zod-to-openapi/dist'
```

I believe it's safe to remove them, was there any specific reason why they were added?
@AGalabov
Copy link
Collaborator

@dmarcato I believe the idea back when we were using the older version was to reexport the types if anyone needed them (without having openapi3-ts as a direct dependency).

The overall idea was that you can do something like:

import type { OpenAPI } from '@asteasolutions/zod-to-openapi';

// use it as:

const schema: OpenAPI.SchemaObject = { ... ]

I don't think it was ever "requested" and it being something strictly important so I'd be down to remove it. However I am kind of confused why you would get such an error in the first place.

Seeing that you mentioned:

you can just import directly from the openapi3-ts dependency without going through this library.

is it possible that you have openapi3-ts as a direct dependency in your project? And is it possible that the two versions are somehow conflicting 🤔

@dmarcato
Copy link
Contributor Author

dmarcato commented Jun 21, 2023

Since openapi3-ts is a dependency of this library, you can already import its definitions like this:

import * as OpenAPIV3 from 'openapi3-ts/oas30';

without having to explicitly add a direct dependency to openapi3-ts.

To be honest I'm not 100% sure why I'm getting that error either, I don't have a direct dependency on openapi3-ts in my project and I see it successfully added in my node-modules. I spent some time debugging but I couldn't really narrow down what is creating issues besides this re-export, and since it doesn't seem necessary this would be a easy win to fix the issue I'm experiencing 😅 I can try to do some more investigations but I might end up down a rabbit hole...

@georgyangelov
Copy link
Collaborator

georgyangelov commented Jun 22, 2023

@dmarcato Hey. I'm not sure importing directly from a transitive dependency is valid 100% of the cases - if the transitive dependency isn't hoisted or there is another bundler which doesn't support it then it may not be possible. Or if you have a linter such as eslint there is a rule usually to have all dependencies declared in package.json.

Can you try changing the lines to:

export type * as OpenAPIV3 from 'openapi3-ts/oas30';
export type * as OpenAPIV31 from 'openapi3-ts/oas31';

Note the addition of type - I'm hoping this would fix your issue as it should not have any runtime effects since it would be only exporting types.

If the above doesn't work - I'm fine with removing the reexports

@samkio
Copy link

samkio commented Aug 31, 2023

I am also seeing the issue of Module not found: Can't resolve 'openapi3-ts/oas30' in '[...]/node_modules/@asteasolutions/zod-to-openapi/dist'

In my case it's where the package that depends on zod-to-openapi is used by another package (I have a schema package that depends directly and then another package that depends on the schema package). All of this is done locally via hoisting in npm workspaces.

As with the requestor I have the package downloaded in my node_modules but it's struggling to find it when running in the dependent package. It could just be some misconfiguration my side but I agree if it's not needed it can be removed.

If a package needs openapi3-ts then it should depend on that package directly, imho.

Workaround to unblock:
https://www.npmjs.com/package/patch-package to remove lines 35,36 in dist/index.js

@dmarcato
Copy link
Contributor Author

Sorry for the delay here - what @samkio described is exactly the same use-case I have and the reason why I created this PR. From the comments here it seems like this was done for an hypothetical ease of use, but seems like it's creating more issues than value. I believe right thing to do is to remove the export and force people to declare an explicit dependency on openapi3-ts.
As a side note, we had a bunch of issues with transitive dependencies not explicitly declared but that we rely on, I strongly believe this is an anti pattern that shouldn't be used.

@AGalabov AGalabov merged commit c99ffec into asteasolutions:master Oct 2, 2023
3 checks passed
@AGalabov
Copy link
Collaborator

AGalabov commented Oct 2, 2023

@dmarcato @samkio - merged the PR and released it as v6.0.0 since it is a breaking change in nature if someone was using the exports. Thanks for the PR and sorry it took so long

@dmarcato dmarcato deleted the patch-1 branch October 5, 2023 22:22
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.

None yet

4 participants