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

Misdocumented getting a verticle configuration #187

Closed
Asfolny opened this issue Sep 8, 2019 · 3 comments
Closed

Misdocumented getting a verticle configuration #187

Asfolny opened this issue Sep 8, 2019 · 3 comments
Labels
bug Something isn't working

Comments

@Asfolny
Copy link

Asfolny commented Sep 8, 2019

Context::config() is typed to be a JS object of {[key: string] : any} | null.
It's not, specifically it's a vert.x/JsonObject.
Implicating that doing config.key will return undefined.

Vert.x is nice enough to provide a getValue, as well as an overload to get a default value should the key not exist though.

Worthy note: this Vertx blog post describes just calling "config()", while calling it as a function creates a java exception, "config" is actually a global...
But it's a java object with a string format of {key=value}, so it's essentially useless.

I did try to look into changing this myself, so I could just submit a PR, but honestly I can't make head nor tail out of how the typing generator works, or how to help it.

@pmlopes pmlopes added the bug Something isn't working label Sep 9, 2019
@pmlopes
Copy link
Contributor

pmlopes commented Sep 9, 2019

You're correct the way verticles work on es4x are slightly different from Java. In Java there's the need to implement an interface in es4x the main script is the "body" of the verticle so the methods config() does not exist, but there are a few globals: config, vertx, process, console, etc...

You also noticed a bug in the generated type definitions, the generated code "assumes" that the Java type JsonObject can be used like a JS Object which sadly isn't true see:

Which means that returned JsonObjects need to follow the Java API not {[key: string] : any}

With vert.x 4 there's a chance that JsonObject will implement the Map interface which would make things simpler (is the first graal issue is fixed) but the best solution would be the fix of the second issue.

In the meantime if there is a simple type definition of the classes:

  • io.vertx.core.json.JsonObject
  • io.vertx.core.json.JsonArray

A fix would be to update the code generator to issue JsonObject instead of {[key: string] : any}

If you want to contribute I can assist you. The steps are:

  1. create the type definitions
  2. Update the generator to use the new types here https://github.com/reactiverse/es4x/blob/develop/codegen/src/main/java/io/reactiverse/es4x/codegen/generator/Util.java#L155 and https://github.com/reactiverse/es4x/blob/develop/codegen/src/main/java/io/reactiverse/es4x/codegen/generator/Util.java#L157
  3. A special note should be done as this is only needed for returned types, in arguments these types are converted by graalvm

@pmlopes
Copy link
Contributor

pmlopes commented Sep 12, 2019

The just released 0.9.1 version (still being synchronized across the maven repos) updates the JsonObject to expose it's properties as a native JS Object. Same applies for JsonArray however this depends on a experimental feature from graal that needs to be enabled with an extra flag:

-Dpolyglot.js.experimental-foreign-object-prototype=true

When upstream (vertx-core) supports json types as Map/List then this issue will be solved by itself, for now this is a workaround. Note that the Array fix will only allow exposing the members using the bracket notation but won't have the full Array prototype implemented (no push/shift/etc...)

@Asfolny
Copy link
Author

Asfolny commented Sep 17, 2019

Been a few days, did try 0.9.1;
Since it fixes this issue, and probably any related to JsonObject (even if only with a experimental flag) this can be closed.

Other than that, I don't have any real input here, I'm not using Java arrays enough to say anything on those, but at least people can know objects work just fine now :D

@Asfolny Asfolny closed this as completed Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants