Fixing resolve logic in serve and lint#19
Conversation
philsturgeon
changed the title
philsturgeon
force-pushed
the
bug/serve-resolve
branch
6 times, most recently
from
6a3410c to
65eae1a
Compare
| class OpenError extends ExtendableError {} | ||
| class ReadError extends ExtendableError {} | ||
|
|
||
| function readFileAsync(filename, encoding) { |
There was a problem hiding this comment.
Copied from resolver, but resolver is going away in a future version as we ship the dependency to a new package. Let's just be ok with this for now.
philsturgeon
force-pushed
the
bug/serve-resolve
branch
from
65eae1a to
08bdd98
Compare
philsturgeon
force-pushed
the
bug/serve-resolve
branch
4 times, most recently
from
10eb272 to
8812a3c
Compare
philsturgeon
force-pushed
the
bug/serve-resolve
branch
from
8812a3c to
afb6d9e
Compare
| source: source | ||
| }; | ||
|
|
||
| options.openapi = openapi; |
There was a problem hiding this comment.
Why not ?
const options = {
resolve: true,
cache: [],
externals: [],
externalRefs: {},
rewriteRefs: true,
origin: source,
source: source,
openapi : openapi
};
There was a problem hiding this comment.
Ha yes. This was code that changed over time, thanks for catching it.
| return loader.loadSpec(file, { resolve: true }); | ||
| } | ||
| catch (error) { | ||
| if (error.name == 'OpenError') { |
There was a problem hiding this comment.
why don't use if(error instanceof OpenError)?
same for ReadError
| if (err) { | ||
| console.log(colors.red + 'Specification schema is invalid.' + colors.reset); | ||
| formatSchemaError(err, options.context); | ||
| process.exit(1); |
There was a problem hiding this comment.
is it ok to exit here? shouldn't throw a validation error or something and be caught in the main func?
There was a problem hiding this comment.
Agreed. There should be some error output for the exit IMO
There was a problem hiding this comment.
Hey, this was confusing as the formatSchemaError was outputting content, so there is always content. I've made those functions return strings, and its a bit more clear now.
const output = formatSchemaError(err, options.context);
console.error(output);
process.exit(1);
perico
left a comment
perico
left a comment
There was a problem hiding this comment.
wrote some minor comments.
well done!
|
Wrote a minor comment which agreed with perico. Looks great. |
dangoosby
left a comment
dangoosby
left a comment
There was a problem hiding this comment.
My comment is based on opinion so I’m approving this PR.
👍
3 participants
When running serve on a $ref-filled schema it fails to load the target. Instead of shows the same index.html so we dont even get a 404, just a invalid response error.
It turns out that lint was also not resolving correctly, so expect to start seeing more linting errors after updating.
For that reason I'm thinking this will be v0.5.0 Don't wanna break things for people on a minor.
Todo