Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

self should be Window or ServiceWorker. #67

Open
nojaf opened this issue Aug 28, 2018 · 23 comments
Open

self should be Window or ServiceWorker. #67

nojaf opened this issue Aug 28, 2018 · 23 comments

Comments

@nojaf
Copy link
Member

nojaf commented Aug 28, 2018

I'm playing around with ServiceWorkers and self is limited to the Window class.

module App.SW

open Fable.Import.Browser
let self = self :?> ServiceWorker

self.addEventListener_install(fun installEvent ->
    printfn "service worker installed, %A" installEvent
)

self.addEventListener_activate(fun activateEvent ->
    printfn "service worker activated, %A" activateEvent
)

self.addEventListener_message(fun ev ->
    printfn "%A" ev.data
)

addEventListener_install doesn't work without the cast.

let [<Global>] self: Window = jsNative

Is not always true.

I'd like to send a PR, how should we solve this? Erased Union Type that capture both Window and ServiceWorker?

@MangelMaxime
Copy link
Member

I would probably do something like: self_window and self_service worker so the API is easy to use. If we do a DU then we need an active pattern, if we use U2<Window, ServiceWorker> there is an "unwrap" to do etc.

@nojaf
Copy link
Member Author

nojaf commented Aug 28, 2018

Good thinking, perhaps we could do the following:

let  [<Obsolete("Use self_window")>][<Global>] self : Window = jsNative
let [<Global>] self_window: Window = jsNative
let [<Global>] self_web_worker : Worker = jsNative
let [<Global>] self_service_worker : ServiceWorker = jsNative

@MangelMaxime
Copy link
Member

I never used self in JavaScript do you have any "simple" documentation or explanation on why/for what it is used ?

@nojaf
Copy link
Member Author

nojaf commented Aug 28, 2018

Service worker events

@MangelMaxime
Copy link
Member

@nojaf Thank you :)

@whitetigle Didn't you work with service worker on a project ?

@alfonsogarciacaro
Copy link
Member

TBH I didn't know self was available in the bindings 😅 When I need it for web workers I usually just declare it.

The proposal looks good, although there may be a couple of points to consider:

  • It's more verbose
  • self is context sensitive, giving all three options may lead some users to believe they can use any one at any time.

AFAIK the current expert of service workers with Fable is @Nhowka ;) Maybe he also has some ideas on how to improve the API?

@alfonsogarciacaro
Copy link
Member

How do they decide what's the correct self in Typescript? I cannot find any example.

@Nhowka
Copy link
Contributor

Nhowka commented Aug 28, 2018

There is more returns for self:

  • Window on normal usage
  • DedicatedWorkerGlobalScope for dedicated web workers
  • SharedWorkerGlobalScope for shared web workers
  • ServiceWorkerGlobalScope for service workers

The only GlobalScope defined on the file is AudioWorkerGlobalScope that I'm not sure if self can return it directly. So as it is, even it being usable, it's not complete yet.

To be fair my opinion is more radical. Keep the current self : Window on Fable.Import.Browser and then have the specializations on their own packages, each one defining self to the expected type.

@nojaf
Copy link
Member Author

nojaf commented Aug 29, 2018

Ok, sounds good. So we would have something like Fable.Import.ServerWorker and this could be used independent of Fable.Import.Browser?

@whitetigle
Copy link
Contributor

@MangelMaxime Yes but I did not migrate my service worker code to f# yet. So no problem with self 😉
I just registered the js code .

I'm too in favor for some specialized package.

@MangelMaxime
Copy link
Member

@whitetigle Oh ok :)

@nojaf
Copy link
Member Author

nojaf commented Aug 29, 2018

@alfonsogarciacaro I believe this is how TypeScript does it.

@Nhowka
Copy link
Contributor

Nhowka commented Aug 29, 2018

@nojaf We could be less aggressive and just add a new module for each one, shadowing self when they are open. Not sure if the bindings take any space on the compiled bundle so having a real extra nuget package could be unnecessary.

I was planning on skipping the bindings entirely and doing a library exposing the service worker common uses via JavaScript files but if you can add the bindings even better.

@nojaf
Copy link
Member Author

nojaf commented Aug 29, 2018

Could you give an example how the shadowing would work? I don't know what happens when two module expose the same member. Last one wins?

@alfonsogarciacaro
Copy link
Member

Thanks for the links @nojaf

Could you give an example how the shadowing would work? I don't know what happens when two module expose the same member. Last one wins?

Yes, last module opened wins. That's why some Option methods are shadowed when opening Fable.Import.Browser :)

Maybe this is an opportunity to clean the Browser (and Core JS) bindings. They were originated from an early Typescript declaration and then it was edited manually over the time. It'd be nice to separate WebWorker and other bindings into their own packages. This could benefit the REPL as Fable.Import.Browser.dll is quite big to download (although I don't know how much space we'll be able to save if any).

I guess we could try running the latest Typescript DOM declarations with ts2fable, overwrite the current bindings and check the diff. But we need to be careful not to overwrite some of the manual changes (e.g. most event calls return a generic now in the bindings so you can just return unit instead of an object).

Is there any volunteer for this? 😸

@Nhowka
Copy link
Contributor

Nhowka commented Aug 29, 2018

The last time I tried to add just the ServiceWorkerGlobalScope and it was a horrible experience. As I added, the missing types would appear minutes later, modifying stuff were unbearably slow and I gave up. Probably it was because I don't have enough memory so the file was too massive for my machine to handle.

Now maybe the tooling is efficient enough for me to try again, but no promises.

@nojaf
Copy link
Member Author

nojaf commented Aug 30, 2018

I'll try to make some time for this as well.

@alfonsogarciacaro
Copy link
Member

Awesome, thank you @nojaf. Please let me know if you need any help.

@nojaf
Copy link
Member Author

nojaf commented Aug 30, 2018

Hmm ran dom.ts throught the latest version of ts2fable.
The file has become incomparable.
image

Maybe extracting Worker stuff from the Browser module is a safer approach.

@Nhowka
Copy link
Contributor

Nhowka commented Aug 30, 2018

Was the file generated with an earlier version of ts2fable? The output is impossible to merge now, but if it was maybe we can reuse that earier version again.

@nojaf
Copy link
Member Author

nojaf commented Aug 31, 2018

Hmm true, but many bugs would have been fixed since that release right?
I'm suggesting the following:

  • create a new file for workers based on ts2fable of lib/lib.webworker.d.ts
  • remove any worker specific stuff from Fable.Import.Browser

@alfonsogarciacaro
Copy link
Member

Yeah, the file was generated with a very early version (also early version of the ts declaration) and there have been many manual tweaks since, so auto merging is going to be difficult :/

Your approach makes sense to me @nojaf. Removing should be easier. If it doesn't take too long it'd be also a good opportunity to remove old APIs like IE-only stuff.

@alfonsogarciacaro
Copy link
Member

As a side note, I need to check but an advantage of using newest ts2fable and dom.ts would be to add more comment documentation to the APIs. We don't have that many at the moment.

But we need to find a way to make it easier to see the diffing. I'll investigate, maybe writing a script to sort interfaces and members within the interface alphabetically.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants