thisdot / open-source

Repository for open source packages
MIT License
34 stars 11 forks source link

RouteConfigService pull url matcher. #72

Open morgnism opened 2 years ago

morgnism commented 2 years ago

Is your feature request related to a problem? Please describe.

RouteConfigService allows me to access the data property either from path object | Resolver but doesn't yet access params from a custom route matcher, for example.

Describe the solution you'd like

If using a custom matcher to return a new UrlSegment, that value is stored in params and accessed via ParamMap. I'd like to be able to either:

  1. access params observable like with ActivatedRoute#ParamsMap
  2. OR have the same service as RouteConfigService
  3. OR a new service other than RouteConfigService

Describe alternatives you've considered

My specific use case required accessing resolved data from a parent route and accessing the returned UrlSegment from the route matcher then performing a withLatestFrom to map the params from ActivatedRoute:

a$ = this.routeConfigService.getLeafConfig('someValue').pipe(
      withLatestFrom(
        this.route.paramMap.pipe(map((params: ParamMap) => params.get('path'))), // 'path' is the UrlSegment
      ).pipe(...)
)

constructor(
  private route: ActivatedRoute,
  private routeConfgiService: RouteConfigService<string, 'someValue'>
) {}

Addtional context

No response

morgnism commented 1 year ago

For the proposal, make a POC where this is an operator that handles the edge case and is reusable:

withLatestFrom(
    this.route.paramMap.pipe(map((params: ParamMap) => params.get('path'))), // 'path' is the UrlSegment
).pipe(...)
morgnism commented 1 year ago

Extend the RouteConfigService into a new service for combining the data from the resolver with the result from a route's URL matcher.

Requirements:

How long will it take to build? What milestones exist (if applicable)?

Milestone 1 - MVP service for URL matching (major version bump 1.3.0)

Est time: 4 hours remaining after the POC

morgnism commented 1 year ago

Comments from Chris:

This looks like a good direction. However, as far as I can see the current POC is a breaking change to the current API. IMO the best approach for now would be to put that logic in a separate service and make that other service combine the data from the current getActivatedRouteConfig with the new data from the paramMap Additionally, I noticed that you only take the paramMap from the “leaf” activated route. This means that if you had couple of nested routes and each of them had a custom route matcher (that only consumed part of the path) you would only have access to paramMap from the last matcher. If you look at this logic it actually takes all the activated routes from the current one to the root and merges all of the values into one object. I think having similar logic for paramMap would be beneficial. We could also just add new methods to the current service but the main objective is to not introduce breaking changes so all the features would need to be added via new methods IMO. Oh… and one final suggestion. With all of the above in mind I think we should use combineLatest the merge the “data” and “paramMap” properties. This makes sure that the data would always be fresh.

morgnism commented 1 year ago

This work should be followed with a blog post about the new service.

This post should include:

dustinsgoodman commented 1 year ago

@morgnism is the estimate based on the issues Chris raised in the POC?