fix(devtools): Changed FakeOutlineTunnel to check for ip and not tag #1666
Conversation
| @@ -24,12 +24,12 @@ export class FakeOutlineTunnel implements Tunnel { | |||
|
|
|||
| constructor(public readonly id: string) {} | |||
|
|
|||
| private playBroken(hostname?: string) { | |||
There was a problem hiding this comment.
Thanks @andreevgy for the contribution. I would suggest to use tag instead of host IP because we might introduce multiple mocked broken/unreachable tunnels.
There was a problem hiding this comment.
Sorry for long reply, with busy with work. But tag does not get passed on this level, so it's not accessible and because of it it's broken. Do we want to pass it here?
There was a problem hiding this comment.
Got it, then I think it's OK to use the IP address here. But I would recommend to export them as consts instead of using them as magic strings, and use these consts both here and in main.ts. Maybe:
export const FAKE_BROKEN_HOST = "...";
export const FAKE_UNREACHABLE_HOST = "...";
There was a problem hiding this comment.
I don't understand. If you can specify the hostname, you can just specify a domain name with the corresponding patterns. No need to use IPs, which are not readable. The hostname is a lot clearer.
There was a problem hiding this comment.
For example: "broken.example.com" or "unreachable.example.com".
This is way better, as it lets use have multiple servers and trigger many behaviors, with the name reflecting the behavior.
There was a problem hiding this comment.
@fortuna should I make this change in separate MR in that case?
There was a problem hiding this comment.
Can you clarify what you are trying to accomplish?
You can already test errors by adding servers with the appropriate host names.
There was a problem hiding this comment.
You are right that it can be tested by adding servers with appropriate host names. But there are default mock ones named "broken" and "unreachable" that do not behave like that. That's what I tried to fix first by checking IP address because tag is not passed all the way here. I thought you meant that instead of ip address we can set certain hostname to mock broken server and that's why I am asking if I should open another MR or do changes here. Because default mock servers that I see now first time launching dev environment are "working" just fine.
| @@ -24,12 +24,12 @@ export class FakeOutlineTunnel implements Tunnel { | |||
|
|
|||
| constructor(public readonly id: string) {} | |||
|
|
|||
| private playBroken(hostname?: string) { | |||
There was a problem hiding this comment.
For example: "broken.example.com" or "unreachable.example.com".
This is way better, as it lets use have multiple servers and trigger many behaviors, with the name reflecting the behavior.
Hello!
I was trying to launch project locally to play around and try to implement one of the features requested in the issues here.
I noticed that fake servers do not give any errors when I try to connect to them and looks like it's because host is used not instead of the tag. So I replaced tags with ips for the ease of work.
Hope it helps and I did not miss any bigger context.