google / j2cl

Java to Closure JavaScript transpiler
Apache License 2.0
1.23k stars 145 forks source link

System.getProperty doesn't seem to work #76

Open sgammon opened 4 years ago

sgammon commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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');

2) 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');

3) 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");
  }
}

4) 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 commented 4 years ago

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 commented 4 years ago

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

niloc132 commented 4 years ago

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 commented 4 years ago

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