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

all methods from InternalUtils should be implemented by JsObject #5

Closed
tbenbrahim opened this issue Jul 25, 2017 · 7 comments
Closed

Comments

@tbenbrahim
Copy link

tbenbrahim commented Jul 25, 2017

Is there a reason JsObject does not have all the methods currently hidden in InternalUtils? Adding them would be trivial, for example:

    @JsOverlay
    public final <T> T get(String key) {
        return Utils.get(this, key);
    }

    @JsOverlay
    public final <T> void set(String key, T value) {
        Utils.set(this, key, value);
    }

    @JsOverlay
    public final boolean has(String key){
        return Utils.has(this, key);
    }

   // ...other methods here

    private static final class Utils{
        public static native <T> void set(JObject object, String key, T value)/*-{
            object[key] = value;
        }-*/;

        public static native <T> T get(JObject object, String key)/*-{
            return object[key];
        }-*/;

        public static native boolean has(JObject jObject, String key) /*-{
            return key in object;
        }-*/;

    // ... other methods here

     Utils(){}
    }

This change would also make JsPropertyMap redundant.

At the very least InternalUtils should be renamed and made public, but I think it would be more natural to invoke the methods on the object itself rather than calling static methods in a utility class

@jDramaix
Copy link
Member

JsObject is part of Elemental and is totally generated based on his definition in the extern file. We don't plan to add any other utility methods on generated code.

@gkdn
Copy link
Member

gkdn commented Jul 25, 2017

I didn't understand this one. What is InternalUtils?

@tbenbrahim
Copy link
Author

The problem is that JsInterop cannot generate object[key] or key in object, so the currently shipping version of JsObject is incomplete.
The same is true for the global objects (Window, Navigator, Document), it would be nice to have has(key) for feature detection. I know I can make my own InternalUtils from jsinterop.base, but the solution would be more complete if there was part of the library.

@tbroyer
Copy link
Contributor

tbroyer commented Jul 25, 2017

What's wrong with Js.asPropertyMap(object).get("key") (or ….getAny("key").asXxx()) and Js.asPropertyMap(object).has("key")? Is your concern only that it's a bit verbose?

@tbenbrahim
Copy link
Author

@tbroyer I did not notice because it is not in my version of jsinterop.base:-)
I did find this though, which is even more verbose:

Js.<Any>cast(object).asPropertyMap().has("key")

I guess it is ok for library code or code generation, but not sure this is what you want to expose to users.
Ideally:

if (window.has("clipboardData"){
   // IE 6-10 clipboardData.setData
else if (navigator.has("Clipboard"){
  // new Clipboard API, write()
} else if (document.has("execCommand"){
   // create hidden text area, select text, execCommand
} else {
    // must be using Lynx or Mosaic
}

is far clearer than

if (Js.<Any>cast(window).asPropertyMap().has("clipboardData"){
   // IE 6-10 clipboardData.setText
else if (Js.<Any>cast(navigator).asPropertyMap().has("Clipboard"){
  // new Clipboard API, setText
} else if (Js.<Any>cast(document).asPropertyMap().has("execCommand"){
   // create hidden text area, select text, execute query
} else {
    // must be using Lynx or Mosaic
}

@tbroyer
Copy link
Contributor

tbroyer commented Jul 25, 2017

https://static.javadoc.io/com.google.jsinterop/base/1.0.0-beta-2/jsinterop/base/Js.html#asPropertyMap-java.lang.Object-

BTW, you wouldn't expose any of that to users. You'd wrap this in a library and/or use JsInterop.

@JsProperty(name="clipboardData", namespace="window")
static native Object getClipboardData();

@JsProperty(name="clipboard", namespace="navigator")
static native Object getNavigatorClipboard();

@JsProperty(name="execCommand", namespace="document")
static native Object getDocumentExecCommand();

(you could use a @JsType and @JsFunction types as return values too; and you could put those properties on @JsType objects and cast accordingly)

if (getClipboardData() != null) {
   // clipboardData.setData
} else if (getNavigatorClipboard() != null) {
  // new Clipboard API, write()
} else if (getDocumentExecCommand() != null) {
  // create hidden text area, select text, execCommand
} else {
  // WTF?
}

@gkdn
Copy link
Member

gkdn commented Jul 25, 2017

It is

Js.asPropertyMap(object).has("key")

not

Js.<Any>cast(object).asPropertyMap().has("key")

On the longer run, we are also considering having a solution for feature-checking that is auto-generated by elemental.

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

No branches or pull requests

4 participants