gwtproject / gwt

GWT Open Source Project
http://www.gwtproject.org
1.51k stars 372 forks source link

Configure jsInterop exporting from .gwt.xml files #9553

Closed realityforge closed 6 years ago

realityforge commented 6 years ago

Currently configuring jsinterop exports involves using the following options on the command line

Most of the other build options are included in .gwt.xml files. Interoperability between javascript libraries and GWT libraries sometimes that certain parts of the codebase need to be strongly correlated. And sometimes the inclusion of a particular .gwt.xml will trigger the need to update the export include/exclude rules.

As for an example where this is useful. As noted in https://github.com/gwtproject/gwt/issues/9552 we are using GWT together with existing javascript debug tools in the react ecosystem. It would be great if we could add some configuration to react/core/ReactDev.gwt.xml that exported appropriate elements so that they are easy to consume in debugger. In production mode (i.e. when you just imported react/core/React.gwt.xml module and did not flick the configuration property to development), no exports would be required.

This is not the only use case, sometimes we want to export a particular widget/component for another team to use and they already require their own .gwt.xml module to do so and it would be good to centralize their requirements there,.

niloc132 commented 6 years ago

Given that .gwt.xml files likely won't exist in GWT 3, this doesn't seem too objectionable - control over "exports" will happen in a different way (see the Closure Compiler's documentation on exports). It looks like generating goog.exportSymbol calls based on goog.define values is totally possible, at least from looking at https://stackoverflow.com/questions/4167519/google-closure-compiler-advanced-remove-code-blocks-at-compile-time, but I'll want to experiment to be sure.

With that said, if anything is marked as isNative=false, it is defined by its library author as "this should be exported" - it is the default, and if you want it to actually be exported, the -generatedJsInteropExports (note the s) should be turned on. I can't think of a reason that any library would ever declare a non-native type (i.e. "for export", and then exclude it - by definition, the exclude feature is to turn off something in someone else's module. Doing that in yet another module sounds like it would lead to dueling disable/enable flags, so I'm not sure how it would make sense to have the -excludeJsInteropExport flag in a given .gwt.xml file, except the main one (and therefore like it already is, at the top level). Deciding not to export something seems to make sense to be up to the compiler flags (not unlike setting style to PRETTY vs OBF, which could also be helpful to have per-module, but the work is done globally). The include flag on a per-module basis suffers from the same problem, that any module declaring an export expects it to be there, so including it again seems superfluous...

On the other hand, I could see a module at least requesting that exports be turned on, or error-ing out if they are not enabled (so as to indicate that the given module will not function correctly).

gkdn commented 6 years ago

In addition to what Colin said; this has been already considered/discussed before:

... Jens Nehlmeier: My first thought was something like in *.gwt.xml files. Then developers can provide one module that is exportable and a second one (inheriting the first) that forbids exporting. A pattern list on command line could probably become clunky.

Goktug Gokdogan: We considered that before but making inherently global decisions locally (in this case deciding exports by creating deps) is problematic in many ways.

For example, what if I depend on X and X depends Y that exports some stuff? Is everybody in the dependency graph exposes two configurations?

What if the source is include by two modules one says exportable=true, the other one says exportable=false? IIRC, we even had a problem with associating src's with their modules.

This also jas annoying effect like, lib A in the dependency graph pulls in exported version of Y, and another lib B depends on exported version of Y but pulls in non-exported version. Both A and B are unrelated and is part of the same app so they work fine. And later stage A stops depending on exports so every user of B starts breaking.

Anyway, we have seen all these kind of issues over time and learned that a global config needs to provided globally in the hard way. I see this as similar to proguard problem and that works reasonable well.

realityforge commented 6 years ago

While I can appreciate that optional exporting of types based on context is not something you want to use in your builds, it is something that can be very useful.

I don't see @JsType(isNative=false) as indicating that the author wants a type exported but as an indication that the class complies with the requirements of exporting to javascript and if the context requires that it be exported, then it can be.

And while the decision to export some types in a particular build configuration or build is a "global" decision, I can not see how it is much different from any of the other global decisions that are made within the .gwt.xml files. The way I see .gwt.xml used in applications is that there is typically a Dev suffixed variant and a Prod suffixed (or unsuffixed) variant that sets up global context (not too dissimilar to the dev versus prod variants of webpack configurations).

And on it introducing implicit dependencies - that could be true but it is no different to where we currently are with .gwt.xml and features like "inherits". If a module has an implicit dependency on module "com.biz.MyModule" but does not declare it but is always compiled together with another module that does explicitly declare the dependency then the compile will work, right up until the time in which second module removed the explicit dependency.

I guess I am struggling to understand why this is different from the other features. If it is a purely a philosophical decision then fine, if it is because GWT 3 will be different then that is also fine.

In the meantime if this feature would never land then is the recommended way to go back to manually exporting types and eschewing @JsType(isNative=false) when this capability is required?

gkdn commented 6 years ago

Yes, you are right that this is not different than other global decisions made locally in a gwt.xml. We think those are bad and problematic so we chose not to introduce another one.

The general principle we followed is; If you need exports, you can tell users that you need generateJsInteropExport flag. If user has concerns about code size, they can use more advanced flags for filtering otherwise they are done. I don't think it warrants a dismiss of non-native JsType but if you think that is not acceptable for your case, you can probably do manual exporting.

niloc132 commented 6 years ago

is the recommended way to go back to manually exporting types and eschewing @JsType(isNative=false) when this capability is required?

I'm not sure I follow, but just in case you are suggesting that a user can skip the @JsType(isNative=false) and just mark the type for export via the compiler flags, no, that will not work.

In general, if I'm not mistaken, the only thing that isNative=false gets you besides the ability to export, is restricting what else you can do with your class - no overloaded @JsMethods, no hidden fields, etc. As ever, your code will be made consistent by the compiler, but indicating that something is not native restricts you from the full set of legal java.

realityforge commented 6 years ago

Roughly what I am saying that in my context objects have to be a certain shape for them to be correctly handled by the javascript library. I was doing that via @JsType(isNative=false) but unfortunately I needed too fine grain control over exports for that to be useful.

The way I "solved" it is to just export my own constructor function and patch the prototype of the class generated by GWT to add functions that use JSNI to call the "real" functions. A bit 🔥 dangerous🔥 I guess but it seems to work 😄. Luckily the only need for this is inc code generated via annotation processor so I can live with that.

An example:

https://github.com/realityforge/react4j/blob/master/processor/src/test/resources/expected/com/example/lifecycle/OverrideLifecycleMethodsComponent_.java#L18-L39

gkdn commented 6 years ago

You might have a modelling problem here.

If you have a JavaScript contract you can represent that with a isNative=true interface. isNative=true or JsFunction doesn't require generateJsInteropExports and your Java objects that are implementing that contract will conform to the contract. In the example you have given, you simply need to extract an interface for ComponentLifeCycle and mark is with a native JsType.

realityforge commented 6 years ago

I didn't think of that but I guess I could extract a JsType(isNative=true) per component that selectively exposes implemented lifecycle methods and implementing that for components. I may try that. Thanks!

I am still stuck with manually patching javascript function object to implement the equivalent of static fields in ES6 but less magic is better.

niloc132 commented 6 years ago

A @JsType(isNative=true) will also get static fields if I'm not mistaken.

Only downside is that any java methods will have to be @JsOverlay, and won't be seen at all by your JS code. As long as they only mutate the object itself though, that seems safe enough...

Other downside: you can't turn this "off" to leave "dev mode", but then again, dev mode could be set to -style=PRETTY for readability as well.

gkdn commented 6 years ago

A @JsType(isNative=true) will also get static fields if I'm not mistaken.

The effect of isNative here is about subclasses fulfilling a javascript contract but you cannot enforce a static contract through inheritance; so this is not useful for static context for solving this problem.