Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Add bindings for Navigator #194

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

forrest-akin
Copy link

No description provided.

@yawaramin
Copy link
Collaborator

Thanks! Will review soon.

Copy link
Collaborator

@yawaramin yawaramin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for this PR. It looks like some of the bindings here are to things that are experimental (e.g.Navigator.connection) or non-standard (Navigator.buildID, Navigator.oscpu). Let's leave out these non-standard or experimental bindings (it's reasonable to comment them out here since you've already done the work). We can revisit them later. Also I realize that could apply to sendBeacon as well but it looks like it has fairly good browser support so let's leave it in with a doc comment that it's still an experimental API.

Also can you add the appropriate @since ... doc comments to the relevant modules and bindings, update the changelog, and the package.json version? You can refer to the CONTRIBUTING.md for details. Thanks!

@@ -4,7 +4,6 @@ type frameList; /* array-like, WindowProxy? */
type idleDeadline; /* Cooperative Scheduling of Background Tasks */
type locationbar; /* "bar object" */
type menubar; /* "bar object" */
type navigator;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting this type will break backward-compatibility, so instead can you alias it to the newly-introduced Webapi__Dom__Navigator.t type and deprecate the alias, thx

| `String(string)
]) => bool = "";
[@bs.send.pipe : t] external share : shareOptions => Js.Promise.t(unit) = "";
[@bs.send.pipe : t] external vibrate : (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are only two types of allowed parameters here, can you use two different bindings i.e. vibrate and vibrateArray? That's the typical style used in bs-webapi, and I recommended the @bs.unwrap style for sendBeacon because it takes so many different types that it would have exploded the number of bindings.

[@bs.send.pipe : t] external vibrate : (
[@bs.unwrap] [
| `Int(int)
| `IntList(list(int))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For vibrateArray we need to ensure that the parameter type is array(int), because a Reason array corresponds to a JavaScript array. It can't be a list.

@@ -71,6 +72,7 @@ include Webapi__Dom__Types;
[@bs.val] external document : Dom.document = "document";
[@bs.val] [@bs.scope "window"] external history : Dom.history = "history";
[@bs.val] [@bs.scope "window"] external location : Dom.location = "location";
[@bs.val] [@bs.scope "window"] external navigator : Webapi__Dom__Navigator.t = "navigator";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize there are similar bindings above it, but I really don't think Webapi.Dom.navigator is a sensible place to access a Navigator object from. Since the Webapi.Dom.Window.navigator binding already exists, we don't need another one here.

@@ -0,0 +1,28 @@
open Webapi.Dom;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing these tests. I know it's a bit annoying, but can you also check in the compiled test output? You'll need to do:

git add --force lib/js/tests/Webapi/Dom/Webapi__Dom__Navigator__test.js

@forrest-akin
Copy link
Author

@yawaramin Thanks for pointing me to the CONTRIBUTION page; missed it on my first pass. I think everything should be in order now. Per Can I Use, I omitted properties/methods that are deprecated/obsolete, commented out anything experimental, so all bindings in the module should be stable. lmk if I missed anything!

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

Successfully merging this pull request may close these issues.

2 participants