feat(linking-tool): add support for links tree #39
Conversation
wjanaszek
force-pushed
the
feature/linking-tool
branch
2 times, most recently
from
4a437d6
to
51c3123
Compare
wjanaszek
force-pushed
the
feature/linking-tool
branch
13 times, most recently
from
934c1e2
to
f7d50d8
Compare
|
Work in progress |
wjanaszek
force-pushed
the
feature/linking-tool
branch
7 times, most recently
from
7d18f6e
to
a74b733
Compare
| private findNodeWithPath(linkPath: Link[]): LinksTree { | ||
| let linksTree = { ...this.linksTree }; | ||
| linkPath.forEach(link => { | ||
| linksTree = linksTree[link.type] as LinksTree; |
There was a problem hiding this comment.
I would check if property is really LinksTree to avoid unhandled errors if wrong path is provided.
| embedded?: { id: string }; | ||
| queryParams?: Params; | ||
| skipLocationChange?: boolean; | ||
| [key: string]: any; |
There was a problem hiding this comment.
Is there any reason why is interpolatable parameters map not nested under dedicated property?
There was a problem hiding this comment.
I am not sure, it's an old code from another project. I think we can remove this for now, if some property will be missing, the change has to be done in this library.
There was a problem hiding this comment.
I'm not sure we can simply remove this. I believe it is used to interpolate url params (like "somePath/:id/update") e.g. search for utils.interpolate(url, params). What I mean is to move this params under some prop, e.g.
skipLocationChange?: boolean; params?: { [key: string]: any; }
and then pass only obj.params instead of obj to the interpolation function. Is it more clear now?
| export function convertParamsToQueryParamsString(params: Params): string { | ||
| let result = '?'; | ||
| Object.keys(params).forEach((key: string) => { | ||
| result += `${key}=${params[key]}&`; |
There was a problem hiding this comment.
If particular param value is an array of strings (common case), with this implementation it will be rendered like: "?param1=val1,val2¶m2=...". This kind of multi-value query param is not correctly parsed by Angular then into Params object again (you will get {param1: "val1,val2", ...}). It's better to convert it to the following form instead: "?param1=val1¶m1=val2¶m2=...". This kind of url could be correctly parsed by the Angular then (you will get {param1: ["val1", "val2"], ... }) and is also widely supported by many web frameworks.
| .concat('-routing.module.ts'); | ||
| } | ||
|
|
||
| private static resolveLoadChildrenForIvy(loadChildrenAssignment: PropertyAssignment): string { |
There was a problem hiding this comment.
I think this is not the best name. You could not use Ivy and use dynamic imports for routing at the same time and still this function will be used. I would name this function like "resolveLoadChildrenFromDynamicImport" (and the other function "resolveLoadChildrenFromStringImport") or something similiar.
The other thing is if you should use "resolveLoadChildren" or "resolveChildModule(Path? Name?)" instead, in my opinion it is more descriptive.
| loadChildrenAssignment.getInitializer() as ArrowFunction | ||
| ) | ||
| .split(/([A-Z]?[^A-Z]*)/g) | ||
| .filter(word => !!word && word !== 'Module' && word !== 'Feature'); |
There was a problem hiding this comment.
I'm not sure why do you filter out 'Module' and 'Feature' words here. Many modules have 'Feature' word in their name, so it could break filename of routing module. I can imagine some apps could also have "Module" word somewhere in their module names (not only in the suffix). These are not preserved words netiher in JS, nor in Angular, so I don't think we can simply filter them like that.
| - create some enum with link types, like the following: | ||
| ``` | ||
| export enum LinkType { | ||
| Admin = 'admin', |
There was a problem hiding this comment.
I'm not sure it will work with current implementation. It seems like enum item name and its value must match exactly 1:1, because I think you use item name instead of value when saving items to map/tree.
There was a problem hiding this comment.
Good point, I will fix it.
wjanaszek
force-pushed
the
feature/linking-tool
branch
from
a74b733
to
3fb5237
Compare
Add README.md
wjanaszek
force-pushed
the
feature/linking-tool
branch
from
3fb5237
to
1ae8340
Compare
Add README.md
…Type, with import path.
No description provided.