Main method launcher #198 #200

stevechy · 2023-05-05T20:51:57Z

Implementing a launch method to use in main()

stevechy · 2023-05-05T21:02:24Z

tyrian/js/src/main/scala/tyrian/TyrianAppF.scala

+  /** Launch app instances after DOMContentLoaded.
+    */
+  def launchOnContentLoaded[F[_] : Async](appDirectory: Map[String, () => TyrianAppF[F, _, _]]): Unit = {
+    val documentReady = new js.Promise((resolve, _reject) => {


If the main <script> tag uses module or defer then it looks like the DOM will be ready when the script is loaded. In that case you could call launch directly.

<script type="module" src="/index.hash1.js"></script> <script src="/index.hash2.js" nomodule defer></script>

tyrian/js/src/main/scala/tyrian/TyrianAppF.scala

davesmith00000

This is really good work! I've left some comments and questions for us to work through.

Apologies for taking too long to come back to you, I've been working on another PR that has just landed. Which unfortunately means you'll need to rebase / update this branch 😬 ...I'm hopeful that it shouldn't bother you too much, but you will need some of this:
#197 (comment)

I'm hoping to cut a release soon and I'd really love to include this change!

davesmith00000 · 2023-05-16T07:06:39Z

examples/main-launcher/src/main/scala/example/Main.scala

+      "CounterApp" -> (() => CounterApp),
+      "ChatApp" -> (() => ChatApp)
+    ))
+    println("")


Maybe just ()?

Oh thanks, this it was leftover from debugging, I will remove it

examples/main-launcher/src/main/scala/example/ChatApp.scala

davesmith00000 · 2023-05-16T07:09:53Z

examples/main-launcher/src/main/scala/example/Main.scala

+
+object Main {
+  def main(args: Array[String]): Unit =
+    TyrianAppF.launchOnContentLoaded(Map(


This needs to change slightly, people shouldn't be pulling in TyrianAppF, they should be using TyrianApp from the IO or ZIO modules. Maybe we just add a companion object there that delegates to this thing?

I think also we could find a different name here to launchOnContentLoaded. 🤔

Is TyrianApp.onLoad(Map(..)) descriptive enough?

Would also be good to allow another constructor:

def launchOnContentLoaded[F[_] : Async](appDirectory: (String, () => TyrianAppF[F, _, _]])*): Unit

So that people can omit the Map:

TyrianAppF.launchOnContentLoaded( "CounterApp" -> () => CounterApp, "ChatApp" -> () => ChatApp )

I wonder if it's also possible to leave out the thunk syntax somehow? So you just get:

TyrianAppF.launchOnContentLoaded( "CounterApp" -> CounterApp, "ChatApp" -> ChatApp )

What do you think?

Ok, I have added the companion objects, changed the method name to onLoad with varargs and removed the thunk syntax.

davesmith00000 · 2023-05-16T07:14:30Z

tyrian/js/src/main/scala/tyrian/TyrianAppF.scala

+  private def appElementFlags(tyrianAppElement: dom.HTMLElement): Map[String,String] =
+    val appFlags = for {
+      (dataAttr, attrValue) <- tyrianAppElement.dataset
+      if dataAttr.startsWith("tyrianAppFlag")


Is this prefix a bit large / wordy? In html form it takes up quite a lot of space?
data-tyrian-app-flag-...

I could change it to tyrian-flag or flag ?

tyrian-flag seems nice?

davesmith00000 · 2023-05-16T07:22:48Z

tyrian/js/src/main/scala/tyrian/TyrianAppF.scala

+        case Some(appSupplier) =>
+          appSupplier().launch(tyrianAppElement, appElementFlags(tyrianAppElement))
+        case _ =>
+          // Log a warning message?


Yes. Even just a println would be fine. 👍

examples/main-launcher/index.html

davesmith00000 · 2023-05-16T07:32:24Z

I've approved and run the workflow - please note that we have a couple of flaky http tests at the moment, so if it fails there it isn't you! :)

#199

…an-app container elements

… TyrianApp companion objects.

stevechy · 2023-05-16T15:41:36Z

examples/main-launcher/src/main/scala/example/CounterApp.scala

+    )
+
+  def router: Location => Msg =
+    _ => Msg.NavigateTo


Not sure if I did this correctly... just made the update do nothing

Perfectly valid, although I'd rename the event to Msg.Ignore or Msg.NoOp or something.

Here is another example taken from the IndigoSandbox object, just if you're interested. This one ignores internal links but follows external ones.

def router: Location => Msg = Routing.externalOnly(Msg.NoOp, Msg.FollowLink(_)) def update(model: Model): Msg => (Model, Cmd[Task, Msg]) = case Msg.NoOp => (model, Cmd.None) case Msg.FollowLink(href) => (model, Nav.loadUrl(href))

https://github.com/PurpleKingdomGames/tyrian/blob/main/indigo-sandbox/src/main/scala/example/IndigoSandbox.scala#L22

davesmith00000

Looks great!

I've left more comments: It would be nice to change the data flag name, but everything else is just "if you want to." There's no real blockers here other than it occurs to me that I haven't actually tested it yet, so I should do that when I get a moment, but this is good work and I think it will be appreciated.

davesmith00000 · 2023-05-16T20:21:04Z

examples/main-launcher/src/main/scala/example/CounterApp.scala

+    )
+
+  def router: Location => Msg =
+    _ => Msg.NavigateTo


Perfectly valid, although I'd rename the event to Msg.Ignore or Msg.NoOp or something.

examples/main-launcher/src/main/scala/example/Main.scala

davesmith00000 · 2023-05-16T20:23:43Z

tyrian-io/src/main/scala/tyrian/TyrianApp.scala

+    TyrianAppF.onLoad(appDirectory: _*)
+
+  def launch[F[_] : Async](appDirectory: (String, TyrianAppF[F, _, _])*): Unit =
+    TyrianAppF.launch(appDirectory: _*)


In these versions, you can fix the type to IO and ZIO respectively, rather than F.

davesmith00000 · 2023-05-16T20:25:27Z

tyrian/js/src/main/scala/tyrian/TyrianAppF.scala

+  private def appElementFlags(tyrianAppElement: dom.HTMLElement): Map[String,String] =
+    val appFlags = for {
+      (dataAttr, attrValue) <- tyrianAppElement.dataset
+      if dataAttr.startsWith("tyrianAppFlag")


tyrian-flag seems nice?

tyrian/js/src/main/scala/tyrian/TyrianAppF.scala

davesmith00000 · 2023-05-16T20:34:00Z

examples/main-launcher/src/main/scala/example/CounterApp.scala

+    )
+
+  def router: Location => Msg =
+    _ => Msg.NavigateTo


Here is another example taken from the IndigoSandbox object, just if you're interested. This one ignores internal links but follows external ones.

def router: Location => Msg = Routing.externalOnly(Msg.NoOp, Msg.FollowLink(_)) def update(model: Model): Msg => (Model, Cmd[Task, Msg]) = case Msg.NoOp => (model, Cmd.None) case Msg.FollowLink(href) => (model, Nav.loadUrl(href))

https://github.com/PurpleKingdomGames/tyrian/blob/main/indigo-sandbox/src/main/scala/example/IndigoSandbox.scala#L22

…ap. Update flag data-attr to data-tyrian-flag. Fixed the types to IO and ZIO in tyrian-io and tyrian-zio.

stevechy · 2023-05-17T01:16:29Z

@davesmith00000 thank you for the feedback, I have updated the PR and I think I addressed all the key points. I won't be able to come back to this for a couple of days but hopefully this is in good shape now.

davesmith00000

I've tested it - works great! Thank you very much for all the work. 🙏

stevechy mentioned this pull request May 5, 2023

Proposal: Generic launch example #198

Closed

stevechy commented May 5, 2023

View reviewed changes

tyrian/js/src/main/scala/tyrian/TyrianAppF.scala Outdated Show resolved Hide resolved

davesmith00000 reviewed May 16, 2023

View reviewed changes

davesmith00000 requested review from hobnob and bilki May 16, 2023 07:30

stevechy added 2 commits May 16, 2023 08:17

PurpleKingdomGames#198 : Add a launch method that scans for data-tyri…

17064fc

…an-app container elements

PurpleKingdomGames#198 : Add main-launcher example

3c06daf

stevechy force-pushed the main-method-launcher-gh198 branch from dcaad10 to a00cbfc Compare May 16, 2023 15:19

PurpleKingdomGames#198 : Update launch method name and signature. Add…

48fed4a

… TyrianApp companion objects.

stevechy force-pushed the main-method-launcher-gh198 branch from a00cbfc to 48fed4a Compare May 16, 2023 15:24

stevechy requested a review from davesmith00000 May 16, 2023 15:24

stevechy commented May 16, 2023

View reviewed changes

davesmith00000 reviewed May 16, 2023

View reviewed changes

PurpleKingdomGames#198 : Change TyrianAppF to take both varargs and M…

55cfde7

…ap. Update flag data-attr to data-tyrian-flag. Fixed the types to IO and ZIO in tyrian-io and tyrian-zio.

davesmith00000 approved these changes May 17, 2023

View reviewed changes

davesmith00000 merged commit edfa36f into PurpleKingdomGames:main May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Main method launcher #198 #200

Main method launcher #198 #200

stevechy commented May 5, 2023

stevechy May 5, 2023

davesmith00000 left a comment

davesmith00000 May 16, 2023

stevechy May 16, 2023

davesmith00000 May 16, 2023

davesmith00000 May 16, 2023

davesmith00000 May 16, 2023

stevechy May 16, 2023

davesmith00000 May 16, 2023

stevechy May 16, 2023

davesmith00000 May 16, 2023

davesmith00000 May 16, 2023

stevechy May 16, 2023

davesmith00000 commented May 16, 2023

stevechy May 16, 2023

davesmith00000 May 16, 2023

davesmith00000 May 16, 2023

davesmith00000 left a comment

davesmith00000 May 16, 2023

davesmith00000 May 16, 2023

davesmith00000 May 16, 2023

davesmith00000 May 16, 2023

stevechy commented May 17, 2023

davesmith00000 left a comment

Main method launcher #198 #200

Main method launcher #198 #200

Conversation

stevechy commented May 5, 2023

Choose a reason for hiding this comment

davesmith00000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davesmith00000 commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davesmith00000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevechy commented May 17, 2023

davesmith00000 left a comment

Choose a reason for hiding this comment