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

Reference resolver in v2 #761

Closed
derberg opened this issue Apr 26, 2023 · 13 comments
Closed

Reference resolver in v2 #761

derberg opened this issue Apr 26, 2023 · 13 comments
Labels
enhancement New feature or request help wanted Extra attention is needed stale

Comments

@derberg
Copy link
Member

derberg commented Apr 26, 2023

In Parser v1 we use @apidevtools/json-schema-ref-parser and now I see that v2 uses @stoplight/json-ref-resolver which is marked as deprecated and points to @apidevtools/json-schema-ref-parser 😄

Screenshot 2023-04-26 at 08 36 52

any particular reason for that?

cc @magicmatatjahu @smoya

@derberg derberg added the bug Something isn't working label Apr 26, 2023
@smoya
Copy link
Member

smoya commented Apr 26, 2023

TBH, I didn't know it was deprecated. In fact, it seems we are not the only one.

Besides that, there is a reason we use that resolver. That's the ref resolver that Spectral uses and requires when instantiating it. See https://github.com/stoplightio/spectral/blob/66f769001f045eb841f4fa35821a83ce0b661074/packages/core/src/types/spectral.ts#L5-L7.

It is anything but ideal, but we had to use it. In fact, it has cons such as circular references not being handled which have been fixed with a workaround @magicmatatjahu did.

There is an effort by Stoplight team for using @apidevtools/json-schema-ref-parser instead. In fact, it seems they started using it but in other tools of their ecosystem. There is an open issue (where @magicmatatjahu also commented time ago) stoplightio/spectral#1054.

I think the benefit on using Spectral is way far (better) than having to use this deprecated ref resolver. So i will say we should just deal with it and either 🙏 some day Stoplight finally does the migration or either we do it in their code base.

cc @magicmatatjahu Just in case, check my comment if I'm missing something or I said something wrong, as you were the one who took care of that part ❤️

@derberg
Copy link
Member Author

derberg commented Apr 26, 2023

sorry but is Spectral needs it, it has package.json for it -> https://github.com/stoplightio/spectral/blob/develop/packages/core/package.json#L44

why do we need it in dependencies? just quickly checking, I see that we have https://github.com/asyncapi/parser-js/blob/next-major/src/resolver.ts but it is not clear why we actually need a custom resolvers to pass to Spectral

@smoya
Copy link
Member

smoya commented Apr 27, 2023

it is not clear why we actually need a custom resolvers to pass to Spectral

Allowing custom resolvers is needed since tools like the generator use it in current implementation. See #593

@derberg
Copy link
Member Author

derberg commented Apr 27, 2023

no no, don't get me wrong, I love the idea of custom resolvers and this is my main reason why I do not like stoplight resolver and that we do not use the resolver we used in v1 -> asyncapi/spec#930

@smoya
Copy link
Member

smoya commented Apr 27, 2023

no no, don't get me wrong, I love the idea of custom resolvers and this is my main reason why I do not like stoplight resolver and that we do not use the resolver we used in v1 -> asyncapi/spec#930

We have the option of creating a PR to Spectral with the needed migration for using the @apidevtools/json-schema-ref-parser. We could focus on that for the Parser-JS v2.1.0 maybe?

@derberg
Copy link
Member Author

derberg commented Apr 27, 2023

yup, that should be ok. The issue I linked is something we fix after spec 3.0 release, so still few months ahead.

I wanted to make sure we are not going away from the reference resolver.

btw, looking at the custom resolver issue, circular refs issue, the duck taping we had to do. Also the fact that we need to maintain AsyncAPI rules on our own.....does it mean that officially spectral folks like to write https://www.asyncapi.com/blog/creating-consistency-announcing-asyncapi-spectral-together but in fact there is not much help from their side to fix outstanding issues?

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@derberg
Copy link
Member Author

derberg commented Oct 25, 2023

still a big problem

@char0n
Copy link
Contributor

char0n commented Nov 16, 2023

Yes indeed, very big problem

@kennethaasan
Copy link

I don't have the full context here, but I'm able to run spectral with @apidevtools/json-schema-ref-parser by doing a flavour of:

import path from "node:path";
import { readFile } from "node:fs/promises";
import fs from "node:fs";
import spectralCore from "@stoplight/spectral-core";
import { bundleAndLoadRuleset } from "@stoplight/spectral-ruleset-bundler/with-loader";
import Parsers from "@stoplight/spectral-parsers";
import yaml from "js-yaml";
import { $RefParser } from "@apidevtools/json-schema-ref-parser";
import { JSONSchema } from "../types.js";

const { Spectral, Document } = spectralCore;

const runSpectralOnFile = async (filePath: string) => {
  const spectral = new Spectral();

  const rulesetFilepath = path.join(process.cwd(), ".spectral.yaml");

  spectral.setRuleset(
    await bundleAndLoadRuleset(rulesetFilepath, { fs, fetch }),
  );

  const file = await readFile(filePath, "utf8");

  const yamlFile = yaml.load(file) as JSONSchema;

  const yamlBundle = await $RefParser.dereference(
    `${path.dirname(filePath)}/`,
    yamlFile,
    {},
  );

  return spectral.run(
    new Document(JSON.stringify(yamlBundle), Parsers.Json, filePath),
  );
};

@char0n
Copy link
Contributor

char0n commented Dec 19, 2023

@kennethaasan yep, but you're not able to do that in asyncapi parser-js library (AFAICT) which uses spectral internally.

@jonaslagoni
Copy link
Member

With spectral there are rules that are applied before and after dereferencing. That is why parser currently only uses spectral, cause the input needs to contain references to correctly apply rules.

@smoya smoya added help wanted Extra attention is needed enhancement New feature or request and removed bug Something isn't working labels Mar 12, 2024
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jul 11, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 19, 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 help wanted Extra attention is needed stale
Projects
None yet
Development

No branches or pull requests

5 participants