-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,5 +73,16 @@ export default EmberObject.extend(AbstractLogger, { | |
} | ||
}), | ||
}).reopenClass({ | ||
isSupported: !!window.ReadableStream, | ||
isSupported: !!window.ReadableStream && !isSafari(), | ||
}); | ||
|
||
// 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
// Until this is fixed, Safari needs to be explicitly targeted for poll-based logging. | ||
function isSafari() { | ||
const oldSafariTest = /constructor/i.test(window.HTMLElement); | ||
const newSafariTest = (function(p) { | ||
return p.toString() === '[object SafariRemoteNotification]'; | ||
})(!window['safari'] || (typeof window.safari !== 'undefined' && window.safari.pushNotification)); | ||
return oldSafariTest || newSafariTest; | ||
} |
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