Fix for Ajv global collision bug #2681#2702
Conversation
| } from '../src' | ||
| import { AdapterParams } from '../../memory/node_modules/@feathersjs/adapter-commons/lib' | ||
|
|
||
| const fixtureAjv = new Ajv({ |
There was a problem hiding this comment.
Do we need a separate instance if the default one has been updated?
There was a problem hiding this comment.
We only need to own our Ajv context if we want to combine schemas by making use of schema $ref. As we took away some of the global side effects of calling schema.
We need a separate instance BECAUSE we need to update/mutate it. This update makes the default instance more immutable and side-effect free.
FossPrime
changed the title
|
As to how to better support refs, we could add an addSchema method to the returned class. Which would then allow users to combine schemas with an import tree. This is much closer to the syntax Ajv uses. // users.schema.ts
export const schema = new FeathersSchema()
export const UserSchema = schema.addSchema(...)
const UserResponseSchema = UserSchema.fo(...)
---
// messages.schema.ts
import { schema } from './users.schema.ts'
export const MessageSchema = schema.addSchema(...)In that example UserSchema shares the same Ajv with MessageSchema. FeathersSchema would be a small class that stores an Ajv instance and takes Ajv parameters in it's constructor. This could be implemented side by side with the current API, with a deprecation warning for anyone using the old unsafe API. |
|
I agree on the type coercion - it's mainly intended for query schemas since they generally come in as strings for REST calls. Not sure what the best setup would be. |
2 participants
Summary
Fixes HMR issues with Ajv's schemas #2681
Sandbox with fix applied: https://stackblitz.com/edit/feathers-vite-chat-pcztgp?file=src%2Fschema%2Fusers.schema.ts,schema%2Fsrc%2Fschema.ts
Repo with fix: https://github.com/MarsOrBust24/feathers-schema-freedom
Chat: https://discord.com/channels/509848480760725514/989366511665819698/999577745010999426
Alternatively we could instantiate Ajv inside the constructor, but it would have a performance penalty.
P.S. Type coercion by default seems like a bad idea, it will hide problems until it's too late and make debugging more difficult.