search

ajv-validator / ajv

The fastest JSON schema Validator. Supports JSON Schema draft-04/06/07/2019-09/2020-12 and JSON Type Definition (RFC8927)
https://ajv.js.org
MIT License
13.72k stars 872 forks source link

in (Deno or browsers) and (skypack or esm.sh), standalone compilation fails on recursive schemas #1850

Open cscheid opened 2 years ago

cscheid commented 2 years ago

This report appears specific to using (either of) skypack or esm.sh to get a ES6 module version of ajv. I see the issue in both Deno v1.16.4 and Chrome 96.0.4664.110 (on OS X, though I doubt that matters).

Specifically, a call to safeStringify in the codegen step appears to find a circular structure, causing the generation to fail. On node.js environments, that doesn't happen.

The version of Ajv you are using 8.8.2

The environment you have the problem with Deno or browser, using the library as packaged by skypack, esm.sh.

Your code (please make it as small as possible to reproduce the issue) A minimal example is here. This works on node 17, fails on Deno 1.16 https://runkit.com/cscheid/ajv-non-issue

Results in node.js v8+ In node.js, the standalone source is generated without a problem.

Results and error messages in your platform There is a runtime error:

> const moduleSrc = standalone(ajv, { "navigation-item": "navigation-item" })
    --> starting at object with constructor 'SchemaEnv'
    |     property 'refs' -> object with constructor 'Object'
    --- property 'navigation-item' closes the circle
    at JSON.stringify (<anonymous>)
    at safeStringify (https://cdn.skypack.dev/-/ajv@v8.8.2-oHh4dWmHmt5cH6zohUYk/dist=es2019,mode=imports/unoptimized/dist/compile/codegen/code.js:138:17)
    at interpolate (https://cdn.skypack.dev/-/ajv@v8.8.2-oHh4dWmHmt5cH6zohUYk/dist=es2019,mode=imports/unoptimized/dist/compile/codegen/code.js:131:78)
    at addCodeArg (https://cdn.skypack.dev/-/ajv@v8.8.2-oHh4dWmHmt5cH6zohUYk/dist=es2019,mode=imports/unoptimized/dist/compile/codegen/code.js:91:17)
    at Object._ (https://cdn.skypack.dev/-/ajv@v8.8.2-oHh4dWmHmt5cH6zohUYk/dist=es2019,mode=imports/unoptimized/dist/compile/codegen/code.js:66:7)
    at refValidateCode (https://cdn.skypack.dev/-/ajv@v8.8.2-oHh4dWmHmt5cH6zohUYk/dist=es2019,mode=imports/unoptimized/dist/standalone/index.js:63:35)
    at https://cdn.skypack.dev/-/ajv@v8.8.2-7PXBFgVwJaIVYRDIomxs/dist=es2019,mode=imports/optimized/ajv.js:287:76
    at Set.forEach (<anonymous>)
    at ValueScope._reduceValues (https://cdn.skypack.dev/-/ajv@v8.8.2-7PXBFgVwJaIVYRDIomxs/dist=es2019,mode=imports/optimized/ajv.js:279:12)
    at ValueScope.scopeCode (https://cdn.skypack.dev/-/ajv@v8.8.2-7PXBFgVwJaIVYRDIomxs/dist=es2019,mode=imports/optimized/ajv.js:266:19)

I suspect the answer here to be that there's a miscompilation somewhere in both esm.sh and skypack.dev. It is somewhat unfortunate, though, because ajv itself works excellently in skypack. It would be fantastic to be able to work around this issue if anyone knows how to do that.

cscheid commented 2 years ago

Some progress: I see that in https://github.com/ajv-validator/ajv/blob/1b07663f3954b48892c7210196f7c6ba08000091/lib/compile/codegen/scope.ts, you have the declaration

export class ValueScopeName extends Name {

But in the browser, I can confirm that the line (which compiles from) https://github.com/ajv-validator/ajv/blob/master/lib/compile/codegen/code.ts#L101, in addCodeArg,

else if (arg instanceof Name) code.push(arg)

the check for arg instanceof Name fails for a variable that's pretty clearly a ValueScopeName... So something is breaking that.

cscheid commented 2 years ago

Ok, I'm pretty sure the issue is that both skypack and esm.sh give me slightly different files when importing through ajv/dist/standalone, compared to when importing through ajv. I'm documenting this in case other people are bit by the same problem, and I'll try to report this upstream. You can see that in the stack trace I offered above, by paying attention to the difference in the file paths: ...mode=imports/unoptimized... vs ...mode=imports/optimized...

    at interpolate (https://cdn.skypack.dev/-/ajv@v8.8.2-oHh4dWmHmt5cH6zohUYk/dist=es2019,mode=imports/unoptimized/dist/compile/codegen/code.js:131:78)
    at addCodeArg (https://cdn.skypack.dev/-/ajv@v8.8.2-oHh4dWmHmt5cH6zohUYk/dist=es2019,mode=imports/unoptimized/dist/compile/codegen/code.js:91:17)
    at Object._ (https://cdn.skypack.dev/-/ajv@v8.8.2-oHh4dWmHmt5cH6zohUYk/dist=es2019,mode=imports/unoptimized/dist/compile/codegen/code.js:66:7)
    at refValidateCode (https://cdn.skypack.dev/-/ajv@v8.8.2-oHh4dWmHmt5cH6zohUYk/dist=es2019,mode=imports/unoptimized/dist/standalone/index.js:63:35)
    at https://cdn.skypack.dev/-/ajv@v8.8.2-7PXBFgVwJaIVYRDIomxs/dist=es2019,mode=imports/optimized/ajv.js:287:76
    at Set.forEach (<anonymous>)
    at ValueScope._reduceValues (https://cdn.skypack.dev/-/ajv@v8.8.2-7PXBFgVwJaIVYRDIomxs/dist=es2019,mode=imports/optimized/ajv.js:279:12)
    at ValueScope.scopeCode (https://cdn.skypack.dev/-/ajv@v8.8.2-7PXBFgVwJaIVYRDIomxs/dist=es2019,mode=imports/optimized/ajv.js:266:19)

That's enough for there to be parallel class hierarchies, and then the instanceof check fails.

I imagine you folks didn't foresee this kind of thing happening, and I totally understand if you consider this to not be something worth addressing. But I imagine that using skypack or esm.sh will only become more common over time.

It's worth noting that the two CDN's fail for slightly different reasons. skypack is getting its optimized vs unoptimized build mixed up. But esm.sh is a little more blunt: it just uses esbuild on the package entrypoints, and then of course the classes we get from ajv/dist/standalone and ajv are different; the former carries its own class hierarchy with it almost by design.

Again, I would totally understand if you consider this a "misfeature" of esm.sh, but it would be very nice if the code just worked in these scenarios.

cscheid commented 2 years ago

As I think about this some more, I belive the easiest fix (and I believe this would work for both CDN's) would be to expose standalone as an export in /lib/ajv.ts, which would avoid the double import statements and the multiple definitions.

I'd be happy to contribute a PR.

epoberezkin commented 2 years ago

So, when you import from Ajv and from standalone they are somehow different, even though standalone imports from the same file, but because the path different it maps to another module I guess (and node.js understands it’s the same module because it uses absolute paths in module cash)…

I am note very supportive of exporting standalone from Ajv, as it is not needed by most users… What I think could be done in v9 is to stop using instanceof entirely, and use some other duck typing indication - instanceof is repeatedly biting users in different scenarios….

I will think a bit about it.

epoberezkin commented 2 years ago

And thanks a lot for such detailed report!

cscheid commented 2 years ago

I am note very supportive of exporting standalone from Ajv, as it is not needed by most users…

I totally understand - I just noticed a big number of exports there and thought I'd suggest. In this case, then, I'll wait for v9 to see if that fixes the skypack issues. In the meantime we can work around locally with a patched bundle on our end.

Thanks again for your work on this! (I'm ok with you folks closing this if you'd prefer to as well.)

debo commented 2 years ago

Sorry for bumping this with no useful addition but, in regards to the Deno topic, is there any plan to have AJV published on such platform? It would be nice if you could. Thanks a lot.

oles commented 2 years ago

I too would love to use AJV with Deno.

I've tried hacking support for it with Deno Deploy without success, as AJV generates code, and Deno Deploy does not support that, it seems.

Kreijstal commented 1 year ago

It's not the only project where this has issues with skypack https://github.com/codemirror/dev/issues/1059