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

System.getProperty doesn't seem to work #76

Open
sgammon opened this issue Feb 2, 2020 · 10 comments
Open

System.getProperty doesn't seem to work #76

sgammon opened this issue Feb 2, 2020 · 10 comments

Comments

@sgammon
Copy link
Contributor

sgammon commented Feb 2, 2020

Describe the bug
I'm trying to use System.getProperty to access a Closure --define=, provided via defs. I can't seem to get it to work no matter what I do.

To Reproduce
Example.java:

package app;
import jsinterop.annotations.JsType;

@JsType
public class Config {
  public static String getAppVersion() {
    return System.getProperty("APP_VERSION", "default_java");
  }
}

module.js:

goog.module('app');

/**
 * Defines the app version.
 *
 * @public
 * @define {!string} APP_VERSION
 */
const version = goog.define('APP_VERSION', 'default_js');

exports = {
  version: version
};

config-test.js:

goog.setTestOnly();

const app = goog.require('app');
const Config = goog.require('app.Config');


testSuite({
  testFrameworkVersion() {
    // tests framework version from JS
    assert(!!app.version);
  },

  testCompareVersions() {
    // tests framework version from J2CL
    assertEquals(
        app.version,
        Config.getAppVersion());
  }
});

BUILD.bazel:

package(default_visibility = ["//visibility:public"])
load("@com_google_j2cl//build_defs:rules.bzl", "j2cl_library")
load("@io_bazel_rules_closure//closure:defs.bzl", "closure_js_test")

j2cl_library(
    name = "Config",
    srcs = ["Config.java"],
)

closure_js_test(
    name = name,
    srcs = "config-test.js",
    html = "config_test.html",
    deps = [
      "@io_bazel_rules_closure//closure/library:testing",
      ":Config",
    ],
    defs = [
        "--define=APP_VERSION=abc123",
    ],
)

(config_test.html omitted because it basically just includes the JS).

The build then fails with the following exception:

java.lang.NullPointerException: NAME APP_VERSION 20 [length: 41] [source_file: bazel-out/darwin-dbg/bin/java/app/Config.js.zip!/app/Config.impl.java.js]
	at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:895)
	at com.google.javascript.jscomp.RemoveUnusedCode.getVarForNameNode(RemoveUnusedCode.java:719)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNameNode(RemoveUnusedCode.java:574)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:420)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1122)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseCall(RemoveUnusedCode.java:631)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:348)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1122)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:429)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1122)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseFunction(RemoveUnusedCode.java:1258)
	at com.google.javascript.jscomp.RemoveUnusedCode.access$1200(RemoveUnusedCode.java:93)
	at com.google.javascript.jscomp.RemoveUnusedCode$Continuation.apply(RemoveUnusedCode.java:1586)
	at com.google.javascript.jscomp.RemoveUnusedCode.traverseAndRemoveUnusedReferences(RemoveUnusedCode.java:269)
	at com.google.javascript.jscomp.RemoveUnusedCode.process(RemoveUnusedCode.java:250)
	at com.google.javascript.jscomp.PhaseOptimizer$NamedPass.process(PhaseOptimizer.java:317)
	at com.google.javascript.jscomp.PhaseOptimizer.process(PhaseOptimizer.java:232)
	at com.google.javascript.jscomp.Compiler.performOptimizations(Compiler.java:2418)
	at com.google.javascript.jscomp.Compiler.lambda$stage2Passes$1(Compiler.java:799)
	at com.google.javascript.jscomp.CompilerExecutor$2.call(CompilerExecutor.java:102)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)

Outputs

goog.module('app.Config$impl');

const j_l_Object = goog.require('java.lang.Object$impl');
const $Util = goog.require('nativebootstrap.Util$impl');

class Config extends j_l_Object {
 
 constructor() {
  Config.$clinit();
  super();
  this.$ctor__app_Config__();
 }
 
 $ctor__gust_Config__() {
  this.$ctor__java_lang_Object__();
 }
 /** @return {?string} */
 static getAppVersion() {
  Config.$clinit();
  return $Util.$getDefine("APP_VERSION", "default_java");
 }
 
 static $clinit() {
  Config.$clinit = () =>{};
  Config.$loadModules();
  j_l_Object.$clinit();
 }
 /** @return {boolean} */
 static $isInstance(/** ? */ instance) {
  return instance instanceof Config;
 }
 
 static $loadModules() {}
 
}
$Util.$setClassMetadata(Config, 'app.Config');

exports = Config; 
//# sourceMappingURL=Config.js.map

Bazel version
2.0.0

Expected behavior
I would expect it not to error, firstly. Then, I would expect it to emit abc123 in place of the string in the define.

If I leave the define unspecified, the error disappears, but then, of course, the default string is used, which fails the test.

@sgammon
Copy link
Contributor Author

sgammon commented Feb 2, 2020

I also wanted to say, I did my best to follow along with #1 and the other existing issues, but I found trouble understanding them or applying them to my situation. What I am asking is, what is the supported way of accessing Closure defines in J2CL?

@niloc132
Copy link
Contributor

niloc132 commented Feb 3, 2020

Example.java/Config.java needs a goog.require("APP_VERSION") (... or goog.require('app')? i'm not as good with how closure modules work) to be emitted somehow in its eventual Example.java.impl.js, so that closure's knows where to find that define for Config.java.impl.js. Try adding creating a new file, named either Example.native.js or Config.native.js, and including the line above.


Second guess, due to my lack of familiarity with the effects of modules and exports: you might need to actually ask for System.getProperty("app.version). I'm not 100% on this, since the goog.define mechanism seems to just need it to be mentioned, not directly referenced? In #1, I'm following the code in google/jsinterop-base, where we split a plain goog.provide (no module) out from the native.js+System.getProperty.

@sgammon
Copy link
Contributor Author

sgammon commented Feb 3, 2020

@niloc132 i've also tried injecting the value via a Closure JS file with a @define/goog.define pair, which i then consume via that symbol in J2CL. and it still does not work. but i will give it a try with .native.js.

it is unclear to me if a --define symbol is the same as a symbol defined with @define, and how those two can converge. once a --define is symbolicated, so to speak, with some module/global variable, i would think the J2CL import would work, but then again i don't know how the Util class depicted above plays into things.

it seems to me from reading the code that the Util class mentioned looks for an "object path" matching the define, which would imply a post-symbolication --define, such that only passing --define=some.path.to.be=true would not work unless that path was attached to some symbol. i don't know if my understanding here is correct or not but i'll report back if i figure it out.

@niloc132
Copy link
Contributor

niloc132 commented Feb 3, 2020

You need both .js and .native.js. The .native.js adds the "goog.require" to your transpiled .java output (since you can't write a java import for a js symbol), and the .js file includes the @define'd goog.define(). The value passed to --define is what is returned from the goog.define call - and what is read via System.getProperty (which is transpiled via jsinterop to a call to Util.$getDefine(), which is implemented with goog.getObjectByName()).

Other examples that might help:
provide+define
https://github.com/VueGWT/vue-gwt/blob/master/core/src/main/java/com/axellience/vuegwt/core/client/VueGWT.js
require, to be merged with the output from a .java file:
https://github.com/VueGWT/vue-gwt/blob/master/core/src/main/java/com/axellience/vuegwt/core/client/VueGWT.native.js
System.getProperty in the .java file:
https://github.com/VueGWT/vue-gwt/blob/master/core/src/main/java/com/axellience/vuegwt/core/client/VueGWT.java#L39
Note that in this case, the provided namespace/object isn't actually connected to the name of the property that is actually being defined - plain provide/require dependencies just mean "hey, that file has to come before me", and that can be enough to change global vars.

One more example, in gwt-dom, trying to provide the same legacy "experience" of telling which browser is running (in this case because blink/webkit's RTL setup is ... not as easy to work with as gecko):
https://github.com/jnehlmeier/gwt-dom/blob/master/src/main/resources/org/gwtproject/dom/client/dom.js
https://github.com/jnehlmeier/gwt-dom/blob/master/src/main/resources/org/gwtproject/dom/client/Element%24UserAgentHolder.native.js
https://github.com/jnehlmeier/gwt-dom/blob/master/src/main/java/org/gwtproject/dom/client/Element.java#L53

@gkdn
Copy link
Member

gkdn commented Feb 4, 2020

Sorry didn't read the whole thread yet but taking a quick; seems like you are using goog.module. goog.module doesn't work with @define; pls use goog.provide instead.

@sgammon
Copy link
Contributor Author

sgammon commented Feb 4, 2020

@gkdn / @niloc132 thank you for your help on this, but I still can't get it to work :(

i've updated my sample according to your directions. here are the changes I made

  1. i added Config.native.js:
goog.require('app');
  1. i amended the file module.js:
goog.provide('app');

/**
 * Defines a symbol for the app version.
 *
 * @public
 * @define {!string} app.version
 */
app.version = goog.define('APP_VERSION', 'alpha');
  1. and, unchanged in my Config.java:
@JsType
public class Config {
  /**
   * Retrieve the application version setting.
   *
   * @return Version assigned for the currently-running application.
   **/
  public static String getAppVersion() {
    return System.getProperty("app.version", "alpha");
  }
}
  1. i am building it with these effective targets:
closure_js_library(
  name = "ConfigModule",
  srcs = ["module.js"],
)

j2cl_library(
  name = "Config",
  srcs = [
    "Config.java",
    "Config.native.js",
  ],
  deps = [
    ":ConfigModule",
  ]
)

@gkdn
Copy link
Member

gkdn commented Feb 4, 2020

Pls change
app.version = goog.define('APP_VERSION', 'alpha');
to
app.version = goog.define('app.version', 'alpha');

System.getProperty uses the name passed to goog.define.

Also you can simplify your setup like following:

j2cl_library(
  name = "Config",
  srcs = [
    "Config.java",
    "Config.native.js",
    "module.js",
  ],
  # or you could simply do a glob for *.java and *.js
)

It would be great to put a sample for others to follow as an example; instead of digging for examples from jre or jsnterop.

@sgammon
Copy link
Contributor Author

sgammon commented Feb 5, 2020

I’ll give that a shot and mark up a PR as a sample. thank you again for your help

@niloc132
Copy link
Contributor

https://github.com/google/j2cl/blob/master/docs/best-practices.md#custom-compile-time-code-stripping should be updated to reflect the way that this is expected to work.

@gkdn
Copy link
Member

gkdn commented Jun 13, 2020

Thinking about it; it is easy introduce a Bazel macro to streamline this.

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

3 participants