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

feat(product): add uri resolver #1472

Merged

Conversation

griest024
Copy link
Member

@griest024 griest024 commented Apr 27, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Fixes #1471

@griest024 griest024 added the package: product @daffodil/product label Apr 27, 2021
@griest024 griest024 requested a review from a team as a code owner April 27, 2021 01:33
@lderrickable
Copy link
Contributor

How exactly is this intended to be used? Because the app dev won't know what kind of url the user will give them (either domain/product/id or domain/product/product-name.html). I would expect that a common resolver would be needed to determine what kind of url is used, then that resolver could call productIdResolver.resolve or productUrlResolver.resolve. Is there another plan to handle this?

@griest024
Copy link
Member Author

griest024 commented Apr 27, 2021

a common resolver would be needed to determine what kind of url is used

That's exactly how it works: https://github.com/graycoreio/daffodil/blob/develop/libs/external-router/routing/src/guard/existence.guard.ts

@griest024
Copy link
Member Author

griest024 commented Apr 27, 2021

Although the guard doesn't directly call the resolver, it adds a route to a specific place in the config and then that presumably is sufficient to invoke the correct resolver.

@lderrickable
Copy link
Contributor

Looks to me like the ExistenceGuard just double checks that the wildcard route it is given is resolvable with the backend platform. It then either proceeds with the same route, or changes the route to a predefined "failed resolution path". I think that's all it does.

I don't see what these resolvers have to do with that guard. If the route being resolved was of the form domain/product:id, that wouldn't be a wildcard route and wouldn't even go through the existence guard. Then you'd have both the id and the url resolvers and the app wouldn't know which one to use. If it had the form domain/product/product-name.html, it would go through the existence guard, then it wouldn't know which resolver to use. I still think you'd need a common resolver to choose one resolver or the other in either case.

@griest024
Copy link
Member Author

@lderrickable hang on I'll write a guide. I should have already done this TBH.

@griest024
Copy link
Member Author

@lderrickable #1474

@lderrickable
Copy link
Contributor

@griest024 With the setup outlined in the docs, how would it handle this route: domain/product:id? Seems like it would get to the DaffProductPageUriResolver and always result in a failed request.

Maybe something like this?

@NgModule({
  imports: [
    DaffExternalRouterModule.forRoot({
      failedResolutionPath: 'not-found',
    }, [
      {
        type: 'product:id',
        route: {
          component: MyProductPageComponent,
          ..., // additional route config, but without path
          resolve: {
            product: DaffProductPageIdResolver
          }
        }
      },
      {
        type: 'product/**',
        route: {
          component: MyProductPageComponent,
          ..., // additional route config, but without path
          resolve: {
            product: DaffProductPageUriResolver
          }
        }
      }
    ]),
  ],
})
class AppModule {}

@lderrickable
Copy link
Contributor

lderrickable commented Apr 27, 2021

actually, I'm not sure that would work, since the existence guard/resolver service would make the "resolvable route" a known route and therefore it wouldn't be caught by the product/**. So yea, I still don't see how all this is going to come together

@griest024
Copy link
Member Author

griest024 commented Apr 27, 2021

@lderrickable I'm not sure exactly what your question is. If you're referring to the use case of putting routes in nested trees, that is supported by insertionStrategy. I added some better docs about that to my PR.

Note that type in the config is an enum defined by the platform and is returned in the driver reponse. For example Magento only ever has these three, You seem to be using it like a route path but its not.

lderrickable
lderrickable previously approved these changes Apr 28, 2021
@lderrickable
Copy link
Contributor

lgtm

@damienwebdev damienwebdev merged commit 555d911 into graycoreio:develop May 3, 2021
@griest024 griest024 deleted the feat/product/routing/resolver/uri branch May 4, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: product @daffodil/product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add product uri resolver
3 participants