Skip to content
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

Should we namespace the library with "namespace": "Webapi"? #9

Open
TheSpyder opened this issue Jun 3, 2021 · 4 comments
Open

Should we namespace the library with "namespace": "Webapi"? #9

TheSpyder opened this issue Jun 3, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@TheSpyder
Copy link
Owner

TheSpyder commented Jun 3, 2021

This is an open question, based on some experimental code I wrote (branch namespace) and a discussion in the ReasonML discord.

I found a way to make webapi compile in a namespace, but there are some big caveats.

requestAnimationFrame

These externals won't be able to live in the top level namespace anymore. I think this is fine, because they should go inside Window anyway.
https://github.com/tinymce/rescript-webapi/blob/faa6bf3ac5671bad2d2ec10b46bad0a828ba2335/src/Webapi.re#L32-L36

Dom submodule

The Webapi.Dom module can not be named Dom.re - we lose access to the ReScript standard DOM types which is unacceptable. This leaves two options:

  • Rename the module. It's Dom2 in the branch but that's just a proof of concept. RDom? WDom? The mind boggles at the bike shedding possibilities.
  • Inline the module (arguably it and Canvas should be inlined regardless, the DOM more or less is the webapi).

Dom constants

If we inline the Dom module, the constants it defines will need a new home:
https://github.com/tinymce/rescript-webapi/blob/9508a6377ed61376b3fec60c1bbd363a45ad4085/src/Webapi/Webapi__Dom.re#L70-L73

These are extremely convenient and losing them would cause a lot of headaches. Moving them to a Webapi.Global style module probably won't win us any fans.

Dom types

Everything in the Webapi__Dom__Types module, which is used by the current Webapi.Dom:
https://github.com/tinymce/rescript-webapi/blob/9508a6377ed61376b3fec60c1bbd363a45ad4085/src/Webapi/Webapi__Dom.re#L68

Most of it can be replaced with polymorphic variants now (#7), but not all of it.

@spocke
Copy link
Collaborator

spocke commented Jun 3, 2021

I vote for inlining the Dom into Webapi and moving the animation frame things to Webapi__Dom__Window. I'm guessing it's a separate structure since the webidl files are also separated like this. But in reality the web platform it self doesn't have that namespacing in the browser you don't use the dom prefix there like dom.window.document so the modules would map closer to the real world rather than the idl world. If we don't inline we would have to invent a weird name for it and that might just make it more confusing to use.

@spocke spocke added the enhancement New feature or request label Jun 3, 2021
@TheSpyder
Copy link
Owner Author

Yeah inline is definitely better than a new module name if we pursue this approach. But while moving the animation stuff to window is a good idea, I'm not sure about moving the constants there. We don't want to encourage people migrating from bs-webapi to open Window to restore implicit access to them.

That's why I was thinking Webapi.Global; Window could include Global so they're available in both places.

@illusionalsagacity
Copy link
Contributor

Webapi.Global makes sense to me, it also has the benefit of mimicking WindowOrWorkerGlobalScope

@TheSpyder
Copy link
Owner Author

Ah yes! those properties need to be added too. And making the module WindowOrWorker would be quite annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants