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

Fix for Ajv global collision bug #2681 #2702

Merged
merged 5 commits into from
Aug 30, 2022
Merged

Fix for Ajv global collision bug #2681 #2702

merged 5 commits into from
Aug 30, 2022

Conversation

FossPrime
Copy link
Member

@FossPrime FossPrime commented Jul 21, 2022

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.

} from '../src'
import { AdapterParams } from '../../memory/node_modules/@feathersjs/adapter-commons/lib'

const fixtureAjv = new Ajv({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a separate instance if the default one has been updated?

Copy link
Member Author

@FossPrime FossPrime Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 FossPrime changed the title Fix for Ajv HMR errors#2681 Fix for Ajv global collision bug #2681 Jul 22, 2022
@FossPrime
Copy link
Member Author

FossPrime commented Jul 22, 2022

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.

@daffl
Copy link
Member

daffl commented Jul 25, 2022

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. addSchema sounds interesting. How does this usually work in AJV?

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

Successfully merging this pull request may close these issues.

2 participants