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

Refactor useCheckUrl hook #402

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

jessevanmuijden
Copy link
Contributor

In order to support pre-authorized_code flow I need to extend the code quite a bit (see https://github.com/wwWallet/wallet-frontend/pull/375/files#diff-c903fe0b0494b98415d2f768669e30bce989f793efa8de228deb83308a3affe6R47-R127). There exists some highly encapsulated logic for the authorization code flow in the useCheckUrl hook. Adding the pre-authorized_code flow implementation to this hook would reduce the code quality. In fact, the current credential offer processing logic may need to be restructured to elegantly support both flows. Therefore, I have isolated the credential offer processing into its own component. I have also abstracted out other components that were managed from the top-level App component via the useCheckUrl hook, which should reduce the superfluous re-renders of the app and its component considerably.

@pstamatop pstamatop requested a review from kkmanos November 4, 2024 11:47
@kkmanos
Copy link
Member

kkmanos commented Nov 11, 2024

Thank you for the contribution. This PR however seems to be moving the protocol implementation from the dependency injection model to react components. This is not something that can be easily merged to the master because the dependency injection model is used to distribute tasks to separate developers to speed-up and assist the development procedure when extending the wwWallet functionalities.

Re-factorings put a lot of burden on the core wwWallet development team because we are maintaining more than one wwWallet deployed instance. Hence, those should be done by the core developers given detailed requirements by whoever needs to extend the code, so as to maintain interoperability with all existing deployments.

@jessevanmuijden
Copy link
Contributor Author

jessevanmuijden commented Nov 11, 2024

@kkmanos

This PR however seems to be moving the protocol implementation from the dependency injection model to react components.

The dependency injection model is still used. The protocol implementation was already part of a component (App), but was implemented in a difficult to maintain and extend hook. This PR just removes the need for a hook by moving the parts of the logic to corresponding components.

This is not something that can be easily merged to the master because the dependency injection model is used to distribute tasks to separate developers to speed-up and assist the development procedure when extending the wwWallet functionalities.

I do not understand how having a dependency injection model or not is related to distributing work across developers, but it is not relevant for this PR, because it is using the dependency injection model, just not the original hook. In fact, I think that this PR will actually make it easier to work on the OO architecture, since it is no longer encapsulated in a complicated hook that is called on every page switch.

Hence, those should be done by the core developers given detailed requirements by whoever needs to extend the code, so as to maintain interoperability with all existing deployments.

Can I be part of the core team? 😇

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