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

add refs variable for jtd validation #2316

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erikbrinkman
Copy link
Collaborator

@erikbrinkman erikbrinkman commented Jul 29, 2023

There seems to be some weird behavior with typescript when refs was left to default. By specifying it manually the behavior was fixed.

What issue does this pull request resolve?

fixes #2167

What changes did you make?

  • Add R extends Record<string, unknown> to compile and validate for JTD schemas that resolves the referenced issue.
  • Add tests that failed prior to the change
  • Added --cache to prettier invocation. This is not related to the diff, but a nice quality of life change. I'll remove if requested

Is there anything that requires more attention while reviewing?

  1. If you're cool with adding --cache to prettier
  2. compile and validate have really complicated types. This change only resolved the existing tests, but we know coverage is not 100%, so if anything comes to mind that this might hurt.

with or without cache, prettier was tweaking files that broke typescript. I reverted the changes here to get the pull request in, but it's worth calling out

There seems to be some weird behavior with typescript when refs was left
to default. By specifying it manually the behavior was fixed.

fixes ajv-validator#2167
@erikbrinkman erikbrinkman force-pushed the jtd-refs-compile branch 3 times, most recently from 0c0ae48 to 29168a4 Compare July 30, 2023 17:48
@erikbrinkman
Copy link
Collaborator Author

@epoberezkin something seems to have changed recently, and tests are failing silently at the build stage with:

Trying https://github.com/uhop/node-re2/releases/download/1.20.1/linux-x64-83.br ...
Trying https://github.com/uhop/node-re2/releases/download/1.20.1/linux-x64-83.gz ...
Building locally ...

> re2@1.20.1 rebuild /home/runner/work/ajv/ajv/node_modules/re2
> node-gyp rebuild

make: Entering directory '/home/runner/work/ajv/ajv/node_modules/re2/build'
  CXX(target) Release/obj.target/re2/lib/addon.o
  CXX(target) Release/obj.target/re2/lib/new.o
../lib/new.cc: In static member function ‘static Nan::NAN_METHOD_RETURN_TYPE WrappedRE2::New(Nan::NAN_METHOD_ARGS_TYPE)’:
../lib/new.cc:313:55: error: ‘kHasIndices’ is not a member of ‘v8::RegExp’
  313 |                 hasIndices = bool(flags & v8::RegExp::kHasIndices);
      |                                                       ^~~~~~~~~~~

In general this seems like a version incompatibility between the local version of v8 and re2, but looking at successful builds, it looks like they were able to fetch the appropriate binary, so maybe the fix is getting that request to go through. The location seems to exist, so it must be some firewalling on githubs side.

Otherwise, nor run test-ci works locally for me.

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

Successfully merging this pull request may close these issues.

Type of sibling property of a ref property is unknown when validated via JTDSchemaType
1 participant