Make IDP routes independent of handlers #1159
Conversation
| this.path = path; | ||
| } | ||
|
|
||
| public getPath(): string { |
There was a problem hiding this comment.
Why not just make path public then?
There was a problem hiding this comment.
Yea, I guess I should do that. My mind is still set very hard on "only expose functions" 😄.
There was a problem hiding this comment.
Discovered a good reason to do it. That would turn the InteractionRoute interface into { path: string } which the Components.js generator is going to deconstruct. So for the InteractionHandler constructor
public constructor(route: InteractionRoute, source: InteractionHandler)Components.js no longer expects a "route" entry, but a "route_path" entry.
There was a problem hiding this comment.
That would turn the
InteractionRouteinterface into{ path: string }
Also if read-only (i.e., with getter)? Might be a bug then, @rubensworks?
There was a problem hiding this comment.
Adding readonly to the interface did not have an impact. But it does make sense I think, since we also use interfaces to define input parameters for constructors with a lot of variables and otherwise that would become unfeasible: https://github.com/solid/community-server/blob/d2870e5c8bc2d86b1a43db3a8e039f8b494657da/config/app/setup/handlers/setup.json#L17-L24
You could perhaps expect Components.js to handle this differently when the parameters are defined as readonly but that might cause confusion (and make @rubensworks's life more difficult 😉)
There was a problem hiding this comment.
Interfaces are indeed deconstructed atm. But we could definitely add something like a tsdoc option to disable this.
joachimvh
force-pushed
the
fix/idp-routes
branch
from
e5f19aa
to
5dfa938
Compare
joachimvh
force-pushed
the
fix/idp-routes
branch
from
5dfa938
to
bf5750d
Compare
✍️ Description
This is a minor change to the updated IDP architecture so the classes responsible for the routes are independent of the handlers (and a small HTML fix).
This will make it easier to update the OIDC library to v7 as otherwise there would currently be a cyclical dependency (the consent handlers needs the provider to create classes in v7, but the provider needs the consent path for redirects), and it also is how it should have been done in the first place anyway.