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

Resolve remote schemas #602

Merged
merged 2 commits into from
Jun 3, 2021
Merged

Resolve remote schemas #602

merged 2 commits into from
Jun 3, 2021

Conversation

drwpow
Copy link
Contributor

@drwpow drwpow commented May 19, 2021

Resolves #321 and adds support for:

  $ref: "some-remote-schema.yaml#/components/schemas/Thing

The goal was pretty straightforward, but quite a bit of code had to change. The major change was moving the loader from the CLI into the Node API. Before, the Node API had to receive a JSON object. Now, the Node API can handle a string pointing to a local file or remote URL (README updated). There was a lot more prep work done, but that was merged in #612 to prepare for this PR (and make v3 much easier to patch, if needed).

This PR will act as a bit of an RFC to see if this is preferable before moving forward (see comments for generated schema).


Merging this will mean version 4.0! 🎉 It will disrupt Node.js API users by changing the main method from sync to async. Other than that, no other breaking changes, which means v4 will be backwards-compatible with v3 for all CLI users.

Before merging, I’d like to resolve #607 and #584 in v3. And this feature will be the first release of v4. So this PR will be merged as soon as those are resolved and published.

Last outstanding item before merging: #616. This will follow that!

@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #602 (248d689) into main (a36a728) will increase coverage by 4.99%.
The diff coverage is 91.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #602      +/-   ##
==========================================
+ Coverage   88.53%   93.52%   +4.99%     
==========================================
  Files           9       11       +2     
  Lines         349      510     +161     
  Branches      123      183      +60     
==========================================
+ Hits          309      477     +168     
+ Misses         37       32       -5     
+ Partials        3        1       -2     
Impacted Files Coverage Δ
src/transform/index.ts 87.93% <78.78%> (+8.30%) ⬆️
src/transform/paths.ts 92.30% <83.33%> (+0.30%) ⬆️
src/transform/responses.ts 89.47% <84.00%> (+1.37%) ⬆️
src/utils.ts 94.73% <90.00%> (+2.00%) ⬆️
src/load.ts 92.47% <92.47%> (ø)
src/transform/request.ts 94.44% <94.44%> (ø)
src/transform/schema.ts 97.59% <95.65%> (+0.53%) ⬆️
src/transform/parameters.ts 98.00% <96.55%> (+0.50%) ⬆️
src/index.ts 89.09% <97.43%> (+16.36%) ⬆️
src/transform/headers.ts 100.00% <100.00%> (+72.72%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd37dd1...248d689. Read the comment docs.

export interface operations {}

export interface external {
"https://raw.githubusercontent.com/drwpow/openapi-typescript/main/tests/v3/specs/petstore.yaml": {
Copy link
Contributor Author

@drwpow drwpow May 19, 2021

Choose a reason for hiding this comment

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

This is the basic format of loading remote schemas:

export interface external {
  [namespace]: {
    // … subschema
  }
}

Looking at how json-schema-ref works, it does a really good job of flattening schemas. But since we’re converting to TypeScript, I have some hesitations in trying to flatten and convert to a different language.

While keeping every subschema within is own namespace is verbose, it’s not limiting. So I feel it meets one of the project goals: support any valid OpenAPI schema, no matter how complicated. The big area of concern is conflicts: just because you reference User in a remote schema, doesn’t mean that you want the rest of that remote schema’s definitions to be flattened and possibly conflict with your own. To prevent that, this approach of namespacing everything lets you reference foreign components in every direction without trying to merge objects that may not be intended or even capable of merging.

If this approach disagrees with the spec (e.g. if the spec demands that all names be unique even across remote schemas) I’d be happy to revise it, but I had trouble finding a clear answer on that. To me it seems illogical that a schema would either be valid or invalid based on remote refs, and so the external interface was born, and this syntax.

/** this is a duplicate of spec.yml#components/schemas/Remote1 */
Remote1: string;
Remote2: external["subschema/remote2.yml"]["components"]["schemas"]["Remote2"];
Circular: components["schemas"]["Circular"];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have a circular reference back to the original schema, that’s preserved! 🎉

@drwpow
Copy link
Contributor Author

drwpow commented May 27, 2021

Update: this will be merged and v4 will be cut after #616 is merged as the final release of v3 (for now).

@drwpow drwpow force-pushed the remote-schema branch 4 times, most recently from d2aa96f to 200ae7f Compare June 3, 2021 05:53
@drwpow drwpow merged commit bfe51e3 into main Jun 3, 2021
@drwpow drwpow deleted the remote-schema branch June 3, 2021 06:06
@drwpow
Copy link
Contributor Author

drwpow commented Jun 3, 2021

Merged! We are now officially in v4. But a v3 branch will be made to make patches easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant