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

How to use it properly #49

Closed
JoepKockelkorn opened this issue Sep 23, 2020 · 19 comments
Closed

How to use it properly #49

JoepKockelkorn opened this issue Sep 23, 2020 · 19 comments
Assignees
Labels
question Further information is requested

Comments

@JoepKockelkorn
Copy link

Imagine in my example repo that I were to:

  • generate another feature, say 'feature-b' also with an overview, 'b-overview' mounted on the root route
  • mount 'feature-b' in my app on path 'b'
  • and I would generate the routes again using the schematic nx g @routerkit/core:parse --project app
  • and I then want to point the user from 'a-overview' to 'b-overview' using a routerlink and the generated routes
  • and then do the same the other way around, adding a routerlink on 'b-overview' pointing to 'a-overview' using the generated routes

I would then stumble upon two circular dependency issues, as the libs 'feature-a' and 'feature-b' cannot depend on the project 'app' so I cannot use the typings. Even if you would generate the typings for the feature libs and expose them from the lib itself, you would have a circular dependency between libs 'feature-a' and 'feature-b' as they need to reference the generated typings from each other. When not using the generated typings, this pictured scenario is perfectly possible because they are loosely coupled using some 'magic' strings. How will I still be able to use the typings in this setup (monorepo nx style)?

@JoepKockelkorn JoepKockelkorn added the question Further information is requested label Sep 23, 2020
@JoepKockelkorn
Copy link
Author

Extra visualisation (don't mind the ui libs, they are redundant to describe this issue).

Without using the typings there are no issues:

When using the typings, there are circular dependencies (red arrows):

@tarsisexistence
Copy link
Owner

tarsisexistence commented Sep 23, 2020

oh, it's weird

because types file should be a separate unit which only imports { TypedRoute } from '@routerkit/core' and only exports the type

can you add a commit to reproduce circular dependencies in your example repo?

@JoepKockelkorn
Copy link
Author

@retarsis Sure, it's this commit. When you open up a-overview.component.ts in VSCode, the nx linting rule will tell you about the circular dependency.

The problem in detail

In a monorepo setup like nx, features (libs) can be reused in multiple apps and don't know anything about the app they're used in. So storing the generated typings on app level is not going to work in my depicted scenario: if you want to use a route path of a feature in another feature and vice versa.

On the other hand, storing the routes in the feature lib itself also won't work as this can introduce circular dependencies as I explained in the example above. So I don't see any other solution than storing the route typings in a separate lib with no reference to the feature lib (to keep it loosely coupled). So the feature lib and all other features/apps can use the generated routes without introducing circular dependencies. This introduces a small problem: the generated routes typings can get out of sync with its source.

In the project I'm working on right now, we just have a separate lib with all routes/magic strings and use this lib in both the components (for routerLinks) and in modules for the routing declaration. This has also the downside of this lib sometimes not being used and the lib with the routes being out of sync with the reality.

Sidetrack

While preparing this comment, I also came across a bug regarding lazy loaded modules and empty paths. The types are not correct for those. They are called 'root' while they're just an empty string. When you use those with the routerLink directive they obviously won't work because there is no route with a 'root' path. The correct route path is an empty string.

@tarsisexistence
Copy link
Owner

tarsisexistence commented Sep 23, 2020

@JoepKockelkorn i'll answer for sidetracks

In general, it is so conceived, to have a "root" key for empty paths
I didn’t think that it might not be obvious, so I will definitely think about it a little later, and I hope I’ll come with a solution. As a last resort, I will document it so that there are no misunderstandings.

{
    location: {
        root: TypedRoute<[
            '/',
            'location'
        ]>;

       map: TypedRoute<[
            '/',
            'location',
            'map'
        ]>;
    }
}

as you can see, location has a property root but the final tuple hasn't root path

sorry for this, I'll take some time to brainstorm this, otherwise I'll add additional documentation about this

does it makes sense? did my answer help?

about the main question - I think the problem is solvable, give me a little more time to figure out what is wrong and how it can be done better

@JoepKockelkorn
Copy link
Author

I think I then stumbled upon a bug in the getRoutes function because I can see that 'root' is in the actual rendered href and the routing in my minimal example repo therefore doesn't work. The href of the link on the a-overview is http://localhost:4200/b/root where it should be http://localhost:4200/b. The typing is correct though.

Btw, I get the way you intended it now. The idea is quite okay 😄 but it might clash when the dev actually has a route named 'root'. It might be more dev friendly if routes.b would also have the asString, asArray and toString methods instead of having to do .root, but I don't know the consequences of that.

@tarsisexistence
Copy link
Owner

tarsisexistence commented Sep 23, 2020

  1. root inside href is a bug. Is it available in your example repo? (Then I'll go there to catch 2 errors at once 😅)

  2. now I am actively thinking about the options for renaming the root. Current options: ROOT(preferable) and EMPTY.
    Not quite satisfied yet, I will make a decision in the near future

  3. we apply root only if particular route has siblings

Correct (we keep root because we have /location and /location/map at the same time)

{
    location: {
        root: TypedRoute<[
            '/',
            'location'
        ]>;

       map: TypedRoute<[
            '/',
            'location',
            'map'
        ]>;
    }
}

Wrong (root is totally redundant and eventually shrinks)

{
    location: {
        root: TypedRoute<[
            '/',
            'location'
        ]>;
    }
}

If route has no siblings, then it will be structured in that way:

{
    location: TypedRoute<[ '/', 'location']>;
}

@JoepKockelkorn
Copy link
Author

@retarsis Yes it's available in my repo. Just navigate to localhost:4200/a after yarn ng serve and the link to b-overview has 'root' in it.

@tarsisexistence
Copy link
Owner

@JoepKockelkorn thanks a lot, again 😅

I will come back to this issue tomorrow as there is a lot of other work to be done.

@tarsisexistence
Copy link
Owner

So, both issues are identified and I started working on it.

@tarsisexistence
Copy link
Owner

@JoepKockelkorn I have news

The fix is in master branch for both issues but I didn't release it because we have few issues to fix with parser. I think we will release this in a few days.

sorry for keeping you waiting

but there is more good news
I added you as a contributor to the project because I appreciate what you have done for the project

@JoepKockelkorn
Copy link
Author

@retarsis Nice, thanks! Regarding the waiting, no problem as I don't use it yet. I have to figure out how in a nx setup.

@tarsisexistence
Copy link
Owner

tarsisexistence commented Sep 29, 2020

@JoepKockelkorn about nx repo issue I decided to just generate routes type file in the rootDir because "circular dependency" isn't real (project compiles and only custom lint rule complains). It's just a custom lint rule from nx team when you import anything in the not appropriate way (from app to feature and from feature to app).

and starting from the next release you generated files will be located in the rootDir which means you won't have circular dependencies anymore

@JoepKockelkorn
Copy link
Author

about nx repo issue I decided to just generate routes type file in the rootDir because "circular dependency" isn't real (project compiles and only custom lint rule complains)

How is the circular dependency not real? Even though they're just typings, it's definitely a reference in both ways.

and starting from the next release you generated files will be located in the rootDir which means you won't have circular dependencies anymore

So what happens if you have two apps and you generate the typings for both of them?

@tarsisexistence
Copy link
Owner

How is the circular dependency not real? Even though they're just typings, it's definitely a reference in both ways.

if I'm not mistaken typescript destroys types on compilation
by the way, routes type file doesn't import anything except TypedRoute from node_modules that's why you won't have circularity anyway. Custom lint rule from nx is just a semantic thing to warn.

So what happens if you have two apps and you generate the typings for both of them?

Each generated routes type file has unique project name (you can't have 2 different apps with the same project name)
Let's suppose you have 2 apps with the following names: app1 and app2.
The output of routerkit will be app1.routes.d.ts. and app2.routes.d.ts in the root folder.

@tarsisexistence
Copy link
Owner

tarsisexistence commented Oct 3, 2020

@JoepKockelkorn

just released 0.8.0
please check it out
https://github.com/retarsis/routerkit/releases/tag/0.8.0

@tarsisexistence tarsisexistence self-assigned this Oct 3, 2020
@JoepKockelkorn
Copy link
Author

@retarsis Looking good, nice to see my comments being taken seriously. However, I'm not convinced of the rootDir approach. Referencing the typed routes of a feature module though the generated routes of an app feels dirty, even though it's just typings and will not be bundled in the resulting deployable bundles.

If this package would have the following features then the devs can decide for themselves how to use it properly:

  • Besides generating routes of an app, the tool allows to generate typed routes of a lazy loaded feature module separately
  • Be able to pass a param/config path where to write the generated typings

If this tool would have these options I would use it as following in a nx repo:

  1. Generate separate sibling libs for all my lazy loaded feature libs where all of the typed routings are going to live
  2. Generate the typings using this tool into the generated libs so it can be used (imported) it other features while keeping the loosely coupled characteristic

Generating the typed routings of a feature module is currently not possible as it throws this error:
Can't find tsconfig inside angular.json for feature-a project. An appropriate config name should include 'tsconfig' and '.json'. I don't know if it even is possible, haven't investigated.

@tarsisexistence
Copy link
Owner

@JoepKockelkorn I do appreciate your efforts, therefore we try to help in response

You say interesting ideas, but meanwhile, they are not quite included in the project plans for the 1.0.0 release, and I want to spend time improving the parser and other basic features that are necessary for the 1.0.0 release.

What you propose sounds interesting, and it really makes sense, since I already had experience with workspaces.
But the problem is that we will have to rewrite many things and this will take a significant amount of time.

Therefore, in order not to miss anything, and to separate the problem from the request, I propose to proceed as follows:

  • close this issue as we have resolved several issues related to the original issue
  • create RFC through a new issue item, and describe exactly what is not ok and how it should work
  • attach a link with this issue to the newly created

@JoepKockelkorn does that suit you?

@JoepKockelkorn
Copy link
Author

@retarsis Yeah of course! My priority is not yours, I understand that completely 🙂

@JoepKockelkorn
Copy link
Author

Closing this issue, created an RFC #73

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants