Implement adding event listeners for beforeunload#176
Implement adding event listeners for beforeunload#176TomiS wants to merge 2 commits intoreasonml-community:masterfrom
Conversation
| [@bs.get] external returnValue : t => string = ""; | ||
| /** | ||
| * returnValue cannot be unset because BS cannot (afaik) bind `delete e['returnValue'];` | ||
| * Setting the value to undefined does not cause expected behavior. | ||
| */ | ||
| [@bs.set] external setReturnValue: (t, string) => unit = "returnValue"; |
There was a problem hiding this comment.
returnValuesi actually defined onEvent`, which this inherits from. Could you move it there instead?
It;s also possible to write a function that does a delete, but you'll have to use %raw. Something like this should work I think:
let deleteReturnValue: t => unit = [%raw event => {|
delete event['returnValue']
|}];There was a problem hiding this comment.
Ah, indeed it is defined in Event. I'll move it there and try adding deleting.
tests and comment fix comment
TomiS
force-pushed
the
beforeunload2
branch
from
19da251 to
6b0b54e
Compare
|
Ok, now I changed the returnValue to be inside Event (including set and delete). But I started reading the Event docs more carefully and they mention the value should be a boolean. However in BeforeUnloadEvent docs they talk about non-empty strings. So the same field should contain different type of data depending on the event type, right? Did I mention I love the web platform... |
glennsl
left a comment
glennsl
left a comment
There was a problem hiding this comment.
Hmm, yeah that's a problem. The DOM specification for Event considers returnValue"historical", whatever that means, so perhaps we could get away with having the setter only accept a string, and to have it only on BeforeUnloadEvent.
But either way the getter can return both boolean and string values, so to be safe it needs to account for that. Not sure if we do that anywhere here already, but if we do this ought to mimic that. Otherwise I think using Js.Types.classify and returning a polymorphic variant is an OK approach.
| stopImmediatePropagation(event); | ||
| stopPropagation(event); | ||
| setReturnValue(event, "Test"); | ||
| deleteReturnValue(event); No newline at end of file |
There was a problem hiding this comment.
Can you commit the JS artifacts as well so we can ensure this generates correct code?
3 participants
The intention is to enable using this browser feature:
https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload
There seems to already be a type for Webapi.BeforeUnloadEvent, but not a attach an event listener for it. So this PR adds it.