fastify / fast-uri

Dependency free RFC 3986 URI toolbox
Other
90 stars 7 forks source link

fast-uri still not compatible for AJV #85

Open jasoniangreen opened 4 months ago

jasoniangreen commented 4 months ago

Prerequisites

Fastify version

2.4.0

Plugin version

No response

Node.js version

18.x

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

14.5

Description

Hi there, I know you're all keen to get fast-uri compatible to be used as default in AJV. I setup the following test with the latest version and it still fails while uri-js passes.

const AJV = require('ajv');
const fastUri = require('fast-uri');
const ajv = new AJV({
  uriResolver: fastUri // comment this line to see it works with uri-js
});

const schema = {
  $ref: "#/definitions/Record%3Cstring%2CPerson%3E",
  definitions: {
    Person: {
      type: "object",
      properties: {
        firstName: {
          type: "string",
        },
      },
    },
    "Record<string,Person>": {
      type: "object",
      additionalProperties: {
        $ref: "#/definitions/Person",
      },
    },
  },
};

const data = {
  joe: {
    firstName: "Joe",
  },
};

const validate = ajv.compile(schema); // throws with fast-uri
console.log(validate(data));

Link to code that reproduces the bug

No response

Expected Behavior

Should log true with fast-uri just as it does with uri-js which can you see if you comment out the line that adds fast-uri.

Uzlopak commented 4 months ago

Somehow strange if i fix the #ref to be $ref: "#/definitions/Record<string,Person>", then it passes in runkit

Added the unit tests of uri-js #86 And if we really try to be compatible with uri-js, then we fail by 11 tests...

jasoniangreen commented 4 months ago

Somehow strange if i fix the #ref to be $ref: "#/definitions/Record<string,Person>", then it passes in runkit

Added the unit tests of uri-js #86 And if we really try to be compatible with uri-js, then we fail by 11 tests...

Yeah, I mean, I am confused as to why uri-js even succeeds with the encoded ref but ultimately it does and now we have to continue supporting it.

gurgunday commented 4 months ago

Hmm, I will try to resolve this for good this weekend but we might have to create 2 modes ultimately

@Uzlopak suggested a full rewrite would probably be faster to support url-js

jasoniangreen commented 3 months ago

I have released 8.17.1 of AJV with fast-uri after the recent fixes for browser support and another issue we found. No guarantees there aren't any others but I will stay on top of issues and hopefully if anything else comes up it can be fixed forward.

mcollina commented 3 months ago

Awesome news @jasoniangreen!