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

Custom resolver for external v7 draft $ref #550

Merged
merged 3 commits into from Oct 28, 2018

Conversation

@lukeautry
Copy link
Contributor

commented Oct 27, 2018

This PR does not need to update the CHANGELOG because it's just a local dev environment thing

Overview

Issue #538 covers the problem. json-schema-to-typescript uses json-schema-ref-parser to normalize and dereference json schemas, but the root schema ref (http://json-schema.org/draft-07/schema) requires making some calls to an external URL, which will always fail for offline devs, or in the case of bad connection, will cause launch.sh to slow down.

Changes

json-schema-ref-parser offers a way to create custom resolvers, which means we can add in custom logic to load the draft 7 schema ref locally.

  • json-schema-to-typescript doesn't support passing $RefOptions through dereference in the current version; updated from 5x to 6x
  • Created a custom resolver for this case that just reads from a local copy of the v7 draft

fix #538

@sourcegraph-bot

This comment has been minimized.

Copy link

commented Oct 27, 2018

Thanks for the contribution, @lukeautry!

You should receive feedback on your pull request within a few days. If you haven't already, please read through the contributing guide, and ensure that you've signed the CLA.

Did you run into any issues when creating this PR? Please describe them in an issue so we can make the experience better for the next contributor.

@sqs
Copy link
Member

left a comment

A few minor changes requested. Overall, this looks great! I will personally very much appreciate this when working on airplane WiFi.

@@ -0,0 +1,241 @@
{
"$schema": "http://json-schema.org/draft-07/schema#",

This comment has been minimized.

Copy link
@sqs

sqs Oct 28, 2018

Member

This same schema already exists in schema/json-schema.schema.json. I think it makes sense in this case to use the existing file instead of adding it again here. Feel free to rename schema/json-schema.schema.json to something more specific, such as schema/json-schema-draft-07.schema.json, if you would find that helpful.

This comment has been minimized.

Copy link
@lukeautry

lukeautry Oct 28, 2018

Author Contributor

Nice, didn't see it there. Using that file now and I did rename it to make it a bit more discoverable.

import { ResolverOptions } from 'json-schema-ref-parser';

/**
* If the host is offline, json-schema-ref-parser will fail to fetch

This comment has been minimized.

Copy link
@sqs

sqs Oct 28, 2018

Member

This comment describes what would happen without this resolver, right? With this resolver, program startup will NOT fail when offline because it won't need to fetch the schema, right?

If so, clarify this docstring. Something like:

This resolver allows json-schema-ref-parser to resolve http://json-schema.org/draft-07/schema# (the v7 draft of JSON Schema) offline, which makes it possible to run and develop Sourcegraph offline.

This comment has been minimized.

Copy link
@lukeautry

lukeautry Oct 28, 2018

Author Contributor

Updated 👍

gulpfile.ts Outdated
@@ -70,7 +72,7 @@ export async function webpackServe(): Promise<void> {
// (the related issue https://github.com/webpack-contrib/webpack-serve/issues/238 perhaps
// explains why: the webpack-serve docs incorrectly state that resolving
// `middleware.webpack()` is not necessary).
;(middleware.webpack() as any).then(() => {
; (middleware.webpack() as any).then(() => {

This comment has been minimized.

Copy link
@sqs

sqs Oct 28, 2018

Member

This extra whitespace may fail CI.

This comment has been minimized.

Copy link
@lukeautry

lukeautry Oct 28, 2018

Author Contributor

This got wiped out with the merge conflicts from your upstream changes.

@lukeautry

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

@sqs The only odd thing here is with the changes to src/settings/MonacoSettingsEditor.tsx - I have an editorconfig plugin and tslint plugin (vscode), and I think the changes that were made on save are actually in line with what the rules should be. I can back out the changes if it's going to fail CI but it would be good to understand how to tune my editor if the whitespace is wrong.

demo

@sqs

This comment has been minimized.

Copy link
Member

commented Oct 28, 2018

Changes look great. Yes, the whitespace will fail CI. We use the esbenp.prettier-vscode extension (see list at https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/.vscode/extensions.json) to format this repository according to the prettier rules. I believe it will work when you install that extension. If you haven't used prettier yet, it's like gofmt for TypeScript (and other langs). You can also just run yarn run prettier at the top level. It will fail but it will correct the files' formatting.

I just ran prettier locally and pushed it up to your brach, so no action necessary on your part.

lukeautry and others added some commits Oct 27, 2018

use existing spec file
rename spec file for better discovery
better jsdoc for resolver

@sqs sqs force-pushed the lukeautry:luke/issue-538 branch from 8fc2124 to f32a568 Oct 28, 2018

@sqs

This comment has been minimized.

Copy link
Member

commented Oct 28, 2018

Rebasing. Will merge when green.

@sqs

sqs approved these changes Oct 28, 2018

@lukeautry

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

Extension works, thanks @sqs

@sqs sqs merged commit f40acdd into sourcegraph:master Oct 28, 2018

1 check passed

buildkite/sourcegraph Build #22258 passed (6 minutes, 38 seconds)
Details
@sqs

This comment has been minimized.

Copy link
Member

commented Oct 28, 2018

Thanks!

*/
export const draftV7resolver: ResolverOptions = {
order: 1,
read: () => fs.readFileSync(path.join('__dirname', '../schema/json-schema-draft-07.schema.json')),

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Oct 29, 2018

Member

Does this have to use sync IO?

This comment has been minimized.

Copy link
@lukeautry

lukeautry Oct 29, 2018

Author Contributor

Looking at the docs now, it looks like this actually can support async with an optional callback param. Given the context here (being done in parallel with Gulp) async is better but I think the difference is probably negligible. Can update if you'd like to stay consistent.

// there should be no reason to make network calls during this process,
// and if there are we've broken env for offline devs/increased dev startup time
http: false,
} as $RefParser.Options['resolve'],

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Oct 29, 2018

Member

Why is this cast needed?

This comment has been minimized.

Copy link
@lukeautry

lukeautry Oct 29, 2018

Author Contributor

The typings don't have an index type for these options, so without the cast you'll get a compiler error that draftv7resolver is an unknown property. Personally, I think this is a strange interface on the part of json-schema-ref-parser but it's what exists there now. Ideally, the typings would be updated.

@lukeautry lukeautry deleted the lukeautry:luke/issue-538 branch Oct 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.