Skip to content

add withDebuggerName #60

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: v4.x
Choose a base branch
from
Open

add withDebuggerName #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Fable.Import.RemoteDev.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ module RemoteDev =
port : int
hostname : string
secure : bool
name: string
getActionType : ('msg->obj) option }

type Action =
Expand Down
25 changes: 15 additions & 10 deletions src/debugger.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module Debugger =
let showWarning (msgs: obj list) = JS.console.warn("[ELMISH DEBUGGER]", List.toArray msgs)

type ConnectionOptions =
| ViaExtension
| ViaExtension of name:string
| Remote of address:string * port:int
| Secure of address:string * port:int

Expand All @@ -41,10 +41,11 @@ module Debugger =
hostname = "remotedev.io"
port = 443
secure = true
name = "Elmish"
Copy link

Choose a reason for hiding this comment

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

Magic string "Elmish" used in several places. Here, L141, and L163. Would empty string be a better default?

Copy link
Author

Choose a reason for hiding this comment

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

Well one is for fallback the other is the default. I am not sure, what is best here though.

getActionType = Some getCase }

match opt with
| ViaExtension -> { fallback with remote = false; hostname = "localhost"; port = 8000; secure = false }
| ViaExtension name -> { fallback with remote = false; hostname = "localhost"; port = 8000; secure = false ; name = name}
| Remote (address,port) -> { fallback with hostname = address; port = port; secure = false }
| Secure (address,port) -> { fallback with hostname = address; port = port }
|> connectViaExtension
Expand Down Expand Up @@ -137,7 +138,7 @@ module Program =

let inline withDebuggerCoders (encoder: Encoder<'model>) (decoder: Decoder<'model>) program : Program<'a,'model,'msg,'view> =
let deflater, inflater = getTransformersWith encoder decoder
let connection = Debugger.connect<'msg> Debugger.ViaExtension
let connection = Debugger.connect<'msg> (Debugger.ViaExtension "Elmish")
withDebuggerUsing deflater inflater connection program

let inline withDebuggerAt options program : Program<'a,'model,'msg,'view> =
Expand All @@ -148,12 +149,16 @@ module Program =
with ex ->
Debugger.showError ["Unable to connect to the monitor, continuing w/o debugger"; ex.Message]
program

let inline withDebuggerName (name:string) (program : Program<'a,'model,'msg,'view>) : Program<'a,'model,'msg,'view> =
try
let deflater, inflater = getTransformers<'model>()
let connection = Debugger.connect<'msg> (Debugger.ViaExtension name)
withDebuggerUsing deflater inflater connection program
Copy link

Choose a reason for hiding this comment

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

All the helper functions call withDebuggerUsing. Maybe the name parameter should be there? To avoid a breaking change, could introduce a new fn withNamedDebuggerUsing with name parameter, move current withDebuggerUsing code there. Then change withDebuggerUsing body to just call withNamedDebuggerUsing.

Copy link

Choose a reason for hiding this comment

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

@et1975 Thoughts?

Copy link

Choose a reason for hiding this comment

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

This should also reduce the usage of the default name down to 1 place.

Copy link
Member

Choose a reason for hiding this comment

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

Using says take this connection, at which point, it seems, specifying the name is already too late. Guess we could change the signature of Using to take a function: name -> connection, but that would change the assumptions I originally had when writing this - someone could establish the connection in an arbitrary fashion with regards to options and timing.
I'm onboard with DRY though, there must be a better way to express the default - once.

Copy link

@kspeakman kspeakman Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could name be string option? Looks like Thoth's default behavior is to skip null fields. When name is None, it should emit the same json as before this change. Or we could default it to "Elmish" before connecting. Then there's no need for an extra Using fn.

Copy link

@kspeakman kspeakman Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Or maybe it should be ViaExtension and ViaNamedExtension of name: string. Then no need to worry about default in other fns.

Copy link
Member

Choose a reason for hiding this comment

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

no need to worry about default in other fns.

I was wondering about that, @OnurGumus is naming supported with remote debugger?

Copy link
Author

Choose a reason for hiding this comment

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

I think not; but I am not sure.

with ex ->
Debugger.showError ["Unable to connect to the monitor, continuing w/o debugger"; ex.Message]
program

let inline withDebugger (program : Program<'a,'model,'msg,'view>) : Program<'a,'model,'msg,'view> =
try
let deflater, inflater = getTransformers<'model>()
let connection = Debugger.connect<'msg> Debugger.ViaExtension
withDebuggerUsing deflater inflater connection program
with ex ->
Debugger.showError ["Unable to connect to the monitor, continuing w/o debugger"; ex.Message]
program
withDebuggerName "Elmish" program