web-wallet: Add possibility to serve the web wallet from a sub folder

[ascartabelli]

Resolves #1362

[nortonandreev]

Sorry for the delayed review – I honestly ran away from reviewing it a few days ago. 😆 But should have explained why, so I will do it now instead.

First, it would have helped if we had more information in the issue description – my understanding is that the Wallet is currently served as part of Rusk, so I assume it's in a subfolder – maybe it's not the case. What is the problem that the issue needs to solve and what is needed in a technical aspect to get us there?

From what I can see, the idea is to add a dynamically set base path when a redirection is happening.

I am personally not a fan of the newly proposed components – the AppAnchor and the AppAnchorButton. For example, a button should only cause navigation within the app – if we redirect to an external resource, it should be an anchor, instead of a button. In that sense, having the AppAnchorButton looks redundant to me.

When it comes to the Anchor, if we had the right interface, which allows us to explicitly set if the reference is external or not, it would have been much easier to handle the issue – we could just append the base path if not external, or don't append otherwise. I saw that addBasePath helper handles this based on the href passed, however, then what AppAnchor/Button means at all if one can pass an external link?

What I do like is that the handling of the redirect is handled on a component/navigation level – so that the developer doesn't have to think about it during development (as in, calling a method to append the base path each time a reference is passed to a component/goto function).

Let's agree on the components and if we need the new ones at all, as well as why we need this change in the first place. Then I will have a more careful look at the PR.

[ascartabelli]

Sorry for the delayed review – I honestly ran away from reviewing it a few days ago. 😆 But should have explained why, so I will do it now instead.

First, it would have helped if we had more information in the issue description – my understanding is that the Wallet is currently served as part of Rusk, so I assume it's in a subfolder – maybe it's not the case. What is the problem that the issue needs to solve and what is needed in a technical aspect to get us there?

From what I can see, the idea is to add a dynamically set base path when a redirection is happening.

I am personally not a fan of the newly proposed components – the AppAnchor and the AppAnchorButton. For example, a button should only cause navigation within the app – if we redirect to an external resource, it should be an anchor, instead of a button. In that sense, having the AppAnchorButton looks redundant to me.

It is an anchor: the AnchorButton (and as a consequence the AppAnchorButton) was meant only as a facilitation to have an anchor that appears as a button, as we do have many in our application.

When it comes to the Anchor, if we had the right interface, which allows us to explicitly set if the reference is external or not, it would have been much easier to handle the issue – we could just append the base path if not external, or don't append otherwise. I saw that addBasePath helper handles this based on the href passed, however, then what AppAnchor/Button means at all if one can pass an external link?

The point here was to have the developer not think at all about which component to import, and so I just wrapped the "@dusk/components" into a version specific for the app.
Adding the base path manually or importing the right component manually is too error prone, it's easy to write a wrong test, it's easy for the reviewer to miss it.

[...]

Let's agree on the components and if we need the new ones at all, as well as why we need this change in the first place. Then I will have a more careful look at the PR.

We can discuss about having an AnchorButton or not in the first place, but this is not the issue to tackle this problem.

The point here is: is there a simpler approach to achieve the same?

[nortonandreev]

The point here was to have the developer not think at all about which component to import, and so I just wrapped the "@dusk/components" into a version specific for the app. Adding the base path manually or importing the right component manually is too error prone, it's easy to write a wrong test, it's easy for the reviewer to miss it.

Sorry, I missed that bit that the App components are part of the app's components library, instead of what's supposed to be the shared components. However, even with that, it's still error-prone, because a developer can still import the components from the shared library, especially as no other component follows the convention {App}ComponentName, so it's natural one would try to directly go for ComponentName instead.

The point here is: is there a simpler approach to achieve the same?

I am personally not aware of such – it's combination of calling a method to append the base path and hijacking the goto and modifying it for our needs. When and how these two can be applied is a different matter, but, overall, these are the two approaches I could think of and they have been utilized in the PR.

[ascartabelli]

The point here was to have the developer not think at all about which component to import, and so I just wrapped the "@dusk/components" into a version specific for the app. Adding the base path manually or importing the right component manually is too error prone, it's easy to write a wrong test, it's easy for the reviewer to miss it.

Sorry, I missed that bit that the App components are part of the app's components library, instead of what's supposed to be the shared components. However, even with that, it's still error-prone, because a developer can still import the components from the shared library, especially as no other component follows the convention {App}ComponentName, so it's natural one would try to directly go for ComponentName instead.

But this is a thing that a reviewer can catch easily.
Same applies to importing the wrapped goto and redirect.

The point here is: is there a simpler approach to achieve the same?

I am personally not aware of such – it's combination of calling a method to append the base path and hijacking the goto and modifying it for our needs. When and how these two can be applied is a different matter, but, overall, these are the two approaches I could think of and they have been utilized in the PR.

I was hoping to have some facilitation from the framework / bundler, but, with the combination of the two that we are using I didn't see one.

Unless I missed something.