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

Refactor to better export oas30 and oas31 compatible with cjs #103

Closed
wants to merge 1 commit into from

Conversation

pjmolina
Copy link
Contributor

@pjmolina pjmolina commented Mar 29, 2023

Refactor to better export oas30 and oas31 compatible with cjs
(it will break compatibility in the way we export).

Can you take a look @RobinTail is this solves your use case?

@pjmolina pjmolina self-assigned this Mar 29, 2023
@pjmolina pjmolina changed the title Refactor to better export oas30 and 0as31 compatible with cjs Refactor to better export oas30 and as31 compatible with cjs Mar 29, 2023
@pjmolina pjmolina changed the title Refactor to better export oas30 and as31 compatible with cjs Refactor to better export oas30 and oas31 compatible with cjs Mar 29, 2023
@RobinTail
Copy link
Contributor

I hope so.
In order to make sure, I need to clone and build it.
I'll check try to check ASAP

```

### To use version 3.0 import

From Typescript you can consume it from the library:

```typescript
import { OpenAPIObject } from "openapi3-ts/model/openapi30";
import { OpenApiBuilder } from "openapi3-ts/dsl/openapi-builder30";
import { oas30 } from "openapi3-ts";
Copy link
Contributor

@RobinTail RobinTail Mar 29, 2023

Choose a reason for hiding this comment

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

this approach does not work for some reason

TS2614: Module '"openapi3-ts"' has no exported member 'oas30'. Did you mean to use 'import oas30 from "openapi3-ts"' instead?

Copy link
Contributor

@RobinTail RobinTail Mar 29, 2023

Choose a reason for hiding this comment

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

maybe need to clean cache... please wait....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Looks like a typo on my side.
Can you confirm this works for you?

import oas30 from "openapi3-ts";

Copy link
Contributor

Choose a reason for hiding this comment

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

this would import the whole object (default) giving it the name oas30. Similar to import lib from "openapi3-ts" approach.

image

The object itself is OK.

I believe the export from index.ts should be done in a slighlty different way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I found the reason, see my suggestion below, @pjmolina

Comment on lines +4 to +5
const oas30 = { ...model.oas30, ...dsl.oas30 };
const oas31 = { ...model.oas31, ...dsl.oas31 };
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these two guys should also be exported (not within the "default"), then they will be importable individually, @pjmolina

Suggested change
const oas30 = { ...model.oas30, ...dsl.oas30 };
const oas31 = { ...model.oas31, ...dsl.oas31 };
export const oas30 = { ...model.oas30, ...dsl.oas30 };
export const oas31 = { ...model.oas31, ...dsl.oas31 };

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to try it locally and then I'll confirm.

Copy link
Contributor

@RobinTail RobinTail Mar 29, 2023

Choose a reason for hiding this comment

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

Yes, I confirm, with this approach individual imports do work

image

Copy link
Contributor

@RobinTail RobinTail Mar 29, 2023

Choose a reason for hiding this comment

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

However, this does not solve the import problem for types (from model)

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps these entities (oas30 and oas31) should be namespaces, not constants.
Thus, they could also contain types ("model")

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to take a bit time to prepare a better suggestion

@RobinTail
Copy link
Contributor

RobinTail commented Mar 29, 2023

Ok. I have a proprosition that I believe solves everything. Without namespaces, but using the power of modules.

The approach is based on a different way of combining entities comparing to the one you have currently.

  1. Instead of src/dsl/index.ts and src/model/index.ts there are two files for each module

src/oas30.ts:

export * from './dsl/openapi-builder30';
export * from './model/openapi30';
export { Server, ServerVariable } from './model/server';
export { IExtensionName, IExtensionType, ISpecificationExtension } from './model/specification-extension';

src/oas31.ts:

export * from './dsl/openapi-builder31';
export * from './model/openapi31';
export { Server, ServerVariable } from './model/server';
export { IExtensionName, IExtensionType, ISpecificationExtension } from './model/specification-extension';
  1. Then your src/index.ts file becomes this beauty:
export * as oas30 from "./oas30";
export * as oas31 from "./oas31";
export { Server, ServerVariable } from "./model/server"

no default export, only oas30 , oas31 , Server and ServerVariable
@pjmolina

@RobinTail
Copy link
Contributor

Maybe I should create another PR for that?

RobinTail added a commit to RobinTail/openapi3-ts that referenced this pull request Mar 29, 2023
@pjmolina
Copy link
Contributor Author

Maybe I should create another PR for that?

Sure! Feel free. I will review and comment tomorrow. Thanks

@pjmolina
Copy link
Contributor Author

Rejecting this PR in favour of: PR #104 by @RobinTail

@pjmolina pjmolina closed this Mar 30, 2023
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

2 participants