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

feat(adapter): Add patch data type to adapters and refactor AdapterBase usage #2906

Merged
merged 6 commits into from
Dec 6, 2022

Conversation

daffl
Copy link
Member

@daffl daffl commented Dec 4, 2022

This pull request adds the ability to use a PatchData type in the adapters which may be different than the usual data type.

It also refactors the AdapterBase to remove the confusing and convoluted $ prefixed service methods. Instead an adapter just implements the underscored service methods as before. While for the top level usage backwards compatible this may be a breaking change in some hopefully still uncommon instances but it reduces a lot of the complexity when implementing a database adapter.

You can still extend directly from the adapter class to implement the actual service methods with your own signatures and return types. This will circumvent the legacy query sanitisation which is now done with query schemas anyway.

@netlify
Copy link

netlify bot commented Dec 4, 2022

Deploy Preview for feathers-dove ready!

Name Link
🔨 Latest commit cbcad81
🔍 Latest deploy log https://app.netlify.com/sites/feathers-dove/deploys/638e37913d540f000cc0f2cc
😎 Deploy Preview https://deploy-preview-2906--feathers-dove.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

}

async create(data: D, params?: P): Promise<T>
async create(data: D[], params?: P): Promise<T[]>
async create(data: D | D[], params?: P): Promise<T | T[]> {
if (Array.isArray(data) && !this.allowsMulti('create', params)) {
throw new MethodNotAllowed('Can not create multiple entries')
Copy link
Member

Choose a reason for hiding this comment

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

"Can Not" --> "Cannot"

Copy link
Member

@marshallswain marshallswain left a comment

Choose a reason for hiding this comment

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

Looks good. Will we make sanitizeQuery conditionally run in another PR, or did I miss it in this one?

@daffl daffl marked this pull request as ready for review December 4, 2022 21:47
Copy link
Member

@marshallswain marshallswain left a comment

Choose a reason for hiding this comment

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

New updates look good.

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