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

Dove Support #623

Closed
wants to merge 14 commits into from
Closed

Dove Support #623

wants to merge 14 commits into from

Conversation

forgot
Copy link
Contributor

@forgot forgot commented Apr 24, 2021

  • Tell us about the problem your pull request is solving.
    Updates @feathersjs dependencies to ^5.0.0-pre.3 to work with Dove
  • Are there any open issues that are related to this?
    Not that I could find...
  • Is this PR dependent on PRs in other repos?
    Nope!

This pull request primarily updates the type expectations to match Dove. Due to the removal of the processHooks function, the combine hook had to be reimplemented. It would obviously be better if this remained it's own branch and did not merge with master, but I'm not sure how to create a pull request that does that.

All tests pass with npm test

forgot added 7 commits April 22, 2021 18:08
Re-implemented `processHooks` inside, since that’s all it was calling.
Changed `feathers` import
Added required properties for `HookContext`
Updated typeexpectations for dslint since `Hook` is now an alias
@forgot forgot changed the title Dove Dove Support Apr 28, 2021
@daffl
Copy link
Member

daffl commented May 14, 2021

This is also great, thank you! Probably needs the same Paginate change we discussed the other day and that's now published.

@forgot
Copy link
Contributor Author

forgot commented May 15, 2021

I've bumped everything to 5.0.0-pre.4 and moved the Paginated import back. All tests are still passing.

@forgot
Copy link
Contributor Author

forgot commented Aug 25, 2021

I've bumped everything to 5.0.0-pre.9, and all tests are still passing. I'm guessing all of these will need to be migrated to the new format with backwards compatibility before this is of any real use in Dove.

@daffl
Copy link
Member

daffl commented Aug 25, 2021

I wonder what the best approach would be here. Should all hooks be updated to the new format and provide backwards compatible before and after wrappers or be forward compatible by checking for the next function like in https://github.com/feathersjs/feathers/blob/dove/packages/authentication-local/src/hooks/protect.ts#L16-L18

I'm also wondering if this might be a good time to look at things that can be improved using the new hooks or consolidated. For example, many common hooks for string casing and array processing could just be wrappers for standard Lodash and I think there is also some overlap with @DaddyWarbucks great feathers-fletching.

@fratzinger
Copy link
Collaborator

Could we have a pre-release of this?

@daffl
Copy link
Member

daffl commented Dec 1, 2021

I believe the latest release should work. Does it not?

@fratzinger
Copy link
Collaborator

I want to update my libraries (feathers-casl and feathers-utils) to feathers@5.

image

Screenshot shows type differences between HookContext without generics from feathers@5 that's passed to checkContext of feathers-hooks-common. When checking out the forgot:Dove branch, this error is gone.
(btw. sorry that it's in german)

This was referenced May 16, 2022
@fratzinger fratzinger mentioned this pull request Sep 27, 2022
@fratzinger
Copy link
Collaborator

Closing in favor of #691. Thanks for your work!

@fratzinger fratzinger closed this Sep 27, 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.

3 participants