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(adapter-commons): multiple issues in adapter-commons #2876

Merged
merged 1 commit into from
Dec 4, 2022

Conversation

TheNoim
Copy link
Contributor

@TheNoim TheNoim commented Nov 16, 2022

Summary

(If you have not already please refer to the contributing guideline as described
here
)

  • Tell us about the problem your pull request is solving.
  • Are there any open issues that are related to this? No
  • Is this PR dependent on PRs in other repos? No

If so, please mention them to keep the conversations linked together.

Other Information

This PR solves the following issues:

  • Usage of double Partial which results in the type Partial<Partial<T>>
  • Corrects return of $find in the test fixture of adapter-commons
  • Corrects return of $update in the test fixture of adapter-commons
  • Changes never[] to any[] in service.test.ts of adapter-commons to prevent type error

Feedback

It is hard to find the contributing.md file. I suggest adding a link in the README.md for better visibility.

@netlify
Copy link

netlify bot commented Nov 16, 2022

‼️ Deploy request for feathers-dove rejected.

Name Link
🔨 Latest commit 6732e2c

@@ -51,7 +50,7 @@ export class MethodBase
}

async $update(id: NullableId, _data: Data, _params?: AdapterParams) {
Copy link
Member

Choose a reason for hiding this comment

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

As far as I know, NullableId is wrong for update. Only patch and remove are allowed to have a NullableId. Am I wrong @daffl?

Copy link
Member

Choose a reason for hiding this comment

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

Correct. It should just be Id. If the interface were setup for NullableId, it would mean you could completely replace multiple documents with the same id, which isn't possible.

Copy link
Member

Choose a reason for hiding this comment

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

This was my bad. A bunch of those issues will be fixed in #2906

Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks!

@daffl daffl merged commit 4ff1ed0 into feathersjs:dove Dec 4, 2022
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.

4 participants