-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
UI: Use polling instead of streaming for following logs in Safari #4335
Conversation
Not until the bugs around https are worked out.
4ac1ec0
to
65adc38
Compare
.filter(frame => frame.Data != null && frame.Offset != null); | ||
|
||
if (frames.length) { | ||
frames.forEach(frame => (frame.Data = window.atob(frame.Data))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it you're using for atob
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The browser. No dependency needed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it's just btob
that has unicode issues is it? 👍 cool
}); | ||
|
||
// Fetch streaming doesn't work in Safari yet despite all the primitives being in place. | ||
// Bug: https://bugs.webkit.org/show_bug.cgi?id=185924 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we could completely replicate/use the environment we are seeing the bug, and then put your working test in it to see if it still works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By working test do you mean this branch? As in test this branch via https in Safari to see if this fixes the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it looks like you've reduced the code to something that someone with no knowledge of the nomad-ui can look at, which is super helpful, but that works - so theres no bug there.
If we can see if we can get this on the same environment of where your bug is. That way we can reproduce everything as much as possible but with simpler code, then and try to drill down on exactly what the problem is - I think you mention it could be something to do with differences with the certs? I think the nomad demo site cert uses SNI for example? I doubt its that but would be good if we can check - just ruling things out really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, to replicate the Safari bug. Yes, that would be great, and as you saw I went down that road.
After failing to replicate it, I had to pull myself out of the rabbit hole and focus back on Nomad 😄
I did publish that code though if you wanted to continue exploring it: https://github.com/DingoEatingFuzz/safari-streaming
I hosted it in my DO droplet with a basic Let's Encrypt cert.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Related WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=185924
Despite having all the requisite primitives and APIs, getting a ReadableStream from a fetch response body in Safari doesn't always work.
For the time being, Safari will instead use the polling-based log watcher, much like IE, Edge, and Firefox use.
I also found a bug in the polling-based log watcher and fixed it.