Skip to content

Add support for custom ref resolvers #1307

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

Closed
1 task done
ianwremmel opened this issue Aug 19, 2023 · 8 comments
Closed
1 task done

Add support for custom ref resolvers #1307

ianwremmel opened this issue Aug 19, 2023 · 8 comments
Labels
enhancement New feature or request openapi-ts Relevant to the openapi-typescript library PRs welcome PRs are welcome to solve this issue! stale

Comments

@ianwremmel
Copy link

ianwremmel commented Aug 19, 2023

Description

I'd really like to be able to support relative file refs. The spec appears to allow them and absolute file paths are impractical for my use case.

Proposal

I was able to use this to deal with the first layer of refs:

const rawWithFullPaths = raw.replaceAll(
  /\$ref: '(\..*)'/g,
  (match, p1) => `$ref: 'file://${path.resolve(path.dirname(apiFileName), p1)}'`
);

const parsed = yml.load(rawWithFullPaths) as OpenAPI3;
const schema = await openapiTS(parsed);

but that breaks down if there are nested refs.

The docs sort of imply that passing a cwd should work

Specify current working directory (cwd) to resolve remote schemas on disk (not needed for remote URL schemas)

const parsed = yml.load(raw) as OpenAPI3;
const schema = await openapiTS(parsed, {
  cwd: path.dirname(apiFileName),
});

but that still throws

✘  Can’t resolve "../../../lib/@clc/health/schema/health-check-response.schema.json" from dynamic JSON. Load this schema from a URL instead.

Would it be practical for openapiTS() to accept a resolver function? By default, it wouldn't necessarily need to do anything more than maintain current behavior (though supporting e.g. node module resolve would be nice 🙂), but it would enable folks to write their own resolvers.

Checklist

@ianwremmel ianwremmel added enhancement New feature or request PRs welcome PRs are welcome to solve this issue! openapi-ts Relevant to the openapi-typescript library labels Aug 19, 2023
@ianwremmel
Copy link
Author

Actually, maybe this is just a documentation request. I managed to get the following working:

import $RefParser from 'json-schema-ref-parser';
import type {OpenAPI3} from 'openapi-typescript';
import openapiTS from 'openapi-typescript';

async function foo() {
  const bundled: OpenAPI3 = await $RefParser.bundle(apiFileName);
  const schema = await openapiTS(bundled);
}

My only complaint is I lose all the named objects I used to have via components, but if I also use json2ts to generate standalone types for each schema file, I don't think that's an issue. So, yea, could something like this be added to the docs to explain how to address the relative path issue?

@drwpow
Copy link
Contributor

drwpow commented Aug 19, 2023

Yeah this has been reported in other places; this just seems like a bug. I think there are just more tests that need to be written for path resolution.

I’m open to the general idea of a resolver, but that should only be needed for aliased paths (which AFAIK the spec doesn’t allow like most JS tools do). The cwd option should work on its own. And does in many cases, but not in this one.

I’m also not opposed to either bundling the spec or encouraging using a 3rd-party library. I just haven’t taken that route yet mostly out of simplicity (it’s annoying for people to use) and I haven’t tested a bundling tool well enough to know it wouldn’t break the TS naming pattern this library uses.

All that said, I think resolving the bugs here would be quickest/least disruptive, but again I’m open to all your proposals if that can’t happen for some reason

@ianwremmel
Copy link
Author

I did a lot of reading between opening this ticket and my first comment, so I have new thoughts :)

I’m open to the general idea of a resolver, but that should only be needed for aliased paths (which AFAIK the spec doesn’t allow like most JS tools do).

Agreed, yea, I don't think openapi-typescript needs any special path resolution for things like node style resolution. Instead, folks should use something like what I posted above and pass a resolver to the RefParser.

The cwd option should work on its own. And does in many cases, but not in this one.

Any idea why it wouldn't work in this case?

All that said, I think resolving the bugs here would be quickest/least disruptive, but again I’m open to all your proposals if that can’t happen for some reason

Agreed, I think instead of my original request, these might make more sense:

  1. Of course, fix the cwd bug :)
  2. Add a YMMV doc on how to use RefParser for bundling and custom resolution
  3. Change the relative path error message to mention passing cwd and/or refer to the RefParser docs.

@drwpow
Copy link
Contributor

drwpow commented Aug 19, 2023

Just released 6.5.1 which adds the ability to use the cwd option when passing in JSON objects (that was a good idea! it seems that cwd had been in there for a very long time, unused, and I don’t recall why/when it was added). Updated the docs to reflect that.

Also added a test specifically for your usecase to make sure it’s working as intended.

Hopefully that resolves the issue without the need for an external library or adding a resolver feature, but let me know if the latest version still gives you trouble.

@ianwremmel
Copy link
Author

Thanks! it looks like it's almost working, but I think there's a slight bug.

       Error: ENOENT: no such file or directory, open '/Users/ian/projects/code-like-a-carpenter/apps/lib/@clc/health/schema/health-check-response.schema.json'
           at Object.openSync (node:fs:603:3)
           at Object.readFileSync (node:fs:471:35)
           at load (/Users/ian/projects/code-like-a-carpenter/apps/node_modules/openapi-typescript/dist/index.cjs:709:47)
           at /Users/ian/projects/code-like-a-carpenter/apps/node_modules/openapi-typescript/dist/index.cjs:800:22
           at walk (/Users/ian/projects/code-like-a-carpenter/apps/node_modules/openapi-typescript/dist/index.cjs:436:3)
           at walk (/Users/ian/projects/code-like-a-carpenter/apps/node_modules/openapi-typescript/dist/index.cjs:438:5)
           at walk (/Users/ian/projects/code-like-a-carpenter/apps/node_modules/openapi-typescript/dist/index.cjs:438:5)
           at walk (/Users/ian/projects/code-like-a-carpenter/apps/node_modules/openapi-typescript/dist/index.cjs:438:5)
           at walk (/Users/ian/projects/code-like-a-carpenter/apps/node_modules/openapi-typescript/dist/index.cjs:438:5)
           at walk (/Users/ian/projects/code-like-a-carpenter/apps/node_modules/openapi-typescript/dist/index.cjs:438:5) {
         errno: -2,
         syscall: 'open',
         code: 'ENOENT',
         path: '/Users/ian/projects/code-like-a-carpenter/apps/lib/@clc/health/schema/health-check-response.schema.json'
       }
       

It's trying to load

/Users/ian/projects/code-like-a-carpenter/apps/lib/@clc/health/schema/health-check-response.schema.json

when it should be trying to load

/Users/ian/projects/code-like-a-carpenter/apps/packages/lib/@clc/health/schema/health-check-response.schema.json

My calling code is

const apiFileName = path.resolve(projectRootPath, 'api.yml');
const schema = await openapiTS(raw, {
  cwd: path.dirname(apiFileName),
});

However, this works:

const apiFileName = path.resolve(projectRootPath, 'api.yml');
const schema = await openapiTS(raw, {
  cwd: apiFileName
});

I think the solution here is actually to just rename cwd to filename. Even if the former example worked, it's not really the current working directory, it's the directory the entrypoint lives in. And, keeping the behavior and just chaning the name means folks using it don't need to apply a path.dirname that's already built in.

@drwpow
Copy link
Contributor

drwpow commented Oct 6, 2023

Wanted to point out that the upcoming v7 (#1368) uses Redocly for resolving schemas, which seems to be pretty bulletproof. Would love to know if this is still an issue with that upgrade.

Copy link
Contributor

github-actions bot commented Aug 6, 2024

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

@github-actions github-actions bot added the stale label Aug 6, 2024
Copy link
Contributor

This issue was closed because it has been inactive for 7 days since being marked as stale. Please open a new issue if you believe you are encountering a related problem.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request openapi-ts Relevant to the openapi-typescript library PRs welcome PRs are welcome to solve this issue! stale
Projects
None yet
Development

No branches or pull requests

2 participants