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

Added routing example #195

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

DeamonDev
Copy link

I've added this example for not only educational purposes. Maybe it is good idea to implement routing mechanism in Tyrian using this library? As you can see this is all about sequential folding which could be generalized to Router[T] where T in this example would be Page enum.

@DeamonDev DeamonDev changed the title added routing example Added routing example Apr 30, 2023
@davesmith00000
Copy link
Member

davesmith00000 commented Apr 30, 2023

Hi, thanks for having a go at this, a routing example is needed.

There are some issues here that I'm having a look at, the two big ones are:

  1. You need to include the sbt build changes in the PR, currently you can't build this! I'm adding them locally so I can play with it.
  2. The solution appears incomplete, in that while it will do the initial routing perfectly well, if the page changes the app won't notice. It's quite possible that you weren't intending to show that, but if this is a routing example then most people who try it will come straight back asking "why doesn't it update when I change the page?". 😅

Routing is becoming a bit of a hot topic at the moment, and I think this library could be part of a solution, but I think there's a more fundamental piece of work to do here to make routing easy for people, which is beyond the scope of this PR. I'm tempted to see if I can make that work first. 🤔

Copy link
Member

@davesmith00000 davesmith00000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for having a go at this. I've been travelling today so apologies if any of this comes across as a little blunt. 😄

I'm going to try and tackle the routing problem next since it keeps coming up, and I think this library you're using could be used somewhere in there, and it's an interesting example.

It does however need more work. I fear that if I gave this to the general public, I'd produce more questions than answers in it's current state. 😄

Importantly, you don't need to do that work (yet?) if you don't want to. An example of how to do routing now with the current verison of Tyrian would be great, and there are at least three or four ways to do it that I've seen (all follow the same basic principle, but the details vary). But I do feel like I need to do some work to make this nice and friendly for everyone - at least the core mechanics of it. In that world, I might for example, give you a function you need to define (alongside init and update and the others) that is something like def routing(path: String): Page.Msg, and you could elect to use this url-dsl library to do that, or something else.

@JSExportTopLevel("TyrianApp")
object HelloTyrian extends TyrianApp[Msg, Model]:

val homePath = root / "home"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that the first thing someone sees is a 'Not Found', because by default they're going to land on /, and there are no instructions as to what to do when you get a 404, for example, you could provide links that do work.

@@ -0,0 +1,127 @@
package myorg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this has come from a Mill build, and I don't believe you've tested it within the examples project, because sbt won't compile this code. It's in the wrong folder structure and it doesn't work.

_ => Page.Counter
),
_ => Page.Home
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say I find this syntax really unfriendly and hard to read - I couldn't recommend it to anyone.

What I would want to see is something like (totally made up):

Path(path) match
  case root => Page.Home
  case root / "counter" => Page.Counter
  case _ => Page.NotFound

I've had a quick look at the library you've pulled in and the closest I can find is this: https://github.com/sherpal/url-dsl/blob/master/url-dsl/src/test/scala/urldsl/examples/RouterUseCaseExample.scala#L71

But here, they're actually making up a mini router of there own in order to get the syntax they want.


def init(flags: Map[String, String]): (Model, Cmd[IO, Msg]) =
(
AppState(getPageFromURL(dom.window.location.href), counter = 0),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preferred pattern here would be to run this side effect in a Cmd on the line below, and that would instantly trigger and update.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your replies! I'm totally new into Elm architecture and don't know best patterns yet. I've updated sbt file. When I posted PR I've wrongly thought that the examples are "ad hoc" meaning that they don't belong to any sbt project. That was because I've only looked at build.sbt at root directory and didn't found examples reference. I realized that examples is sbt project on its own after your comment :) So sorry for the confusion.

@DeamonDev
Copy link
Author

Project now compiles :) Also I added navigation between not found page and home page as a side effect.

@DeamonDev
Copy link
Author

I added custom router built on the bones of url-dsl .

@davesmith00000
Copy link
Member

davesmith00000 commented May 4, 2023

Hi @DeamonDev - apologies, I'm not ignoring this! I've been a bit under the weather this week and I'm catching up.

Also I started this on ... Tuesday? Just before I got sick. It's relevant to your interests: #197

UPDATE: https://github.com/PurpleKingdomGames/tyrian/pull/197/files#diff-8c5dd774df58700766b57b940dedc4d8e15a70a778be780b30337c915b50f8bbR22

@davesmith00000 davesmith00000 mentioned this pull request May 6, 2023
@davesmith00000
Copy link
Member

Morning @DeamonDev! OK, the Frontend Routing PR has been merged.

#197

Should be released in the next day or so and then we can revisit this piece of work and hopefully, hopefully, this will make people very happy. 😄

@davesmith00000
Copy link
Member

Hi @DeamonDev - Tyrian 0.7.0 is out with basic frontend routing built-in, but I'm keen to update this example to use it, and to use it in conjunction with url-dsl if you're still interested. Do you want to carry on or would you like a hand? Happy to help. 😄

@DeamonDev
Copy link
Author

Hi @davesmith00000 !

Thanks for info, recently I had very limited time and didnt track the changes. But of course we could try to work on improving this example.

@davesmith00000
Copy link
Member

Hi @davesmith00000 !

Thanks for info, recently I had very limited time and didnt track the changes. But of course we could try to work on improving this example.

Great! Let me know if you need anything. 😄

@DeamonDev
Copy link
Author

@davesmith00000 feel free to edit this example (if you would like to go that route 😅). The change is quite straightforward. Unfortunately my bloop server die for some reason giving me:

[E] Expected compatible Scala.js version [0.6, 1.0], 1.1.13 given

As I said I have very limited time in the upcoming 2 or 3 weeks and definitely don't have time to battle with broken tooling...

@DeamonDev DeamonDev force-pushed the feature/add-more-advanced-routing-example branch from f39c532 to 758eb90 Compare May 23, 2023 07:28
@davesmith00000
Copy link
Member

[E] Expected compatible Scala.js version [0.6, 1.0], 1.1.13 given

Mistyped Scala.js version? 1.1.13 -> 1.13.1?

@davesmith00000
Copy link
Member

I'm out of time this morning but I'll try and have a look later. I've kicked off a CI build in the meantime. Thanks! 👍

@DeamonDev
Copy link
Author

@davesmith00000 yup I misstyped that. It turned out to be my wrong setup. Now its working and I rebased onto main and updated the code :)

@davesmith00000
Copy link
Member

@DeamonDev - I've started refactoring and simplifying but once again I've run out of time. Please review / pull the change.

If you've already done something similar or you feel it isn't right, please feel free to push a revert. 😄

There are still some parts that need work and some parts that actually don't seem to work, but I'll get back to it later unless you beat me to it. 😉

I quite like it though!

@DeamonDev
Copy link
Author

DeamonDev commented May 24, 2023

Looks great! I admit that my code was somewhat chaotic and now it looks much better. I think the url should be updated while walking on the page. It may be achieved as a side effect in update function, but maybe your new routing solution could take care of that?

@edit

I’ve just seen you added Nav.pushLink so you can ignore my comment above ^^

@davesmith00000
Copy link
Member

Still going, pushed another commit, but unfortunately something is not working now. Clearly I broke it, but I'm not sure how and I've run out of time again for today. Any ideas @DeamonDev?

image

@DeamonDev
Copy link
Author

I also dont have too much time today but I think it may be related to the fact that you're pushing page.address to Nav.pushUrl in line 33. This is of course /counter which is not a valid URL. So my first try would be to change that line into Nav.pushUrl(s"localhost:1234${page.address}).

@davesmith00000
Copy link
Member

Hmmm. So the problem with that notion is that it means you cannot use matchRawUrl, because you simply cannot guarantee that everyone is going to supply full Urls, in fact most won't. People are going to write a(href := "/counter") and that's absolutely valid.

I'm currently locked in a sort of dread fascination with this thing. I'm making some progress.. I think... by having implemented a custom UrlStringParserGenerator based on Tyrian's LocationDetails type (which is far more relaxed than JS's Url type - the thing throwing the exception above).

...but I have to say I'm increasingly thinking this library may not be worth the hassle. It looks very promising, but I'm finding it difficult to work with for some reason. I suspect I'm just using it incorrectly or something..

I'll have one more go in the morning, I'm suspicious of the recursive function that does the matching.

@davesmith00000
Copy link
Member

I've pushed another change, this now works apart from the back button. For some reason we're getting double entries into the history stack, so pressing back does nothing. If you remove the pushUrl then we're still getting some history entries, which is weird. Need to take another look.

I've rewritten a lot of the code, but in fact I don't think it was strictly necessary. The main problem that took me ages to work out, was that if one of your routes is just root (as in, the homepage) then a matchPath on that will match EVERYTHING, because root exists within every path.

Nearly there...

@davesmith00000
Copy link
Member

@DeamonDev this all works correctly now, if you'd like to take a look.

One problem I've noticed is that I've made a choice in the way the routing code works that was perhaps a mistake. Needs some thought, but I'd welcome your opinion:

When you click a button, you generate a Msg directly and to have the address bar updated, you then need to use Nav.pushUrl to make the change.

When you use an anchor tag:

  1. If it's an external link and you want to follow it, you need to use Nav.loadUrl.
  2. If it's an internal link, we auto-magically update the address bar for you... but do nothing else!

Point 2 was meant to be helpful, but now I think the inconsistency is just confusing (it confused me!). In this example I've had to add a new message type to account for navigation from a button vs an anchor tag.

I suspect, I should just take out the automagical address bar update and leave you to manage this stuff yourself, to be consistent - because it doesn't just update the address bar but the history too. What do you think?

@DeamonDev
Copy link
Author

My opinion is to be as consistent as possible. Maybe it would be possible to add navigateTo property which could be added to Button as well as to anchor tag?

@davesmith00000
Copy link
Member

#204

@DeamonDev DeamonDev closed this Jun 6, 2023
@DeamonDev DeamonDev reopened this Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants