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

Add support for GWT #140

Merged
merged 1 commit into from
Nov 25, 2017
Merged

Conversation

dojcsak
Copy link
Contributor

@dojcsak dojcsak commented Nov 5, 2017

Add support for GWT compile.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 87.5% when pulling 5a3d464 on dojcsak:add-support-for-gwt into 68ab037 on mkarneim:master.

@mkarneim
Copy link
Owner

mkarneim commented Nov 8, 2017

Hi dojcsak,

Thank you for your contribution.

I would love to add it to PB, but I guess we need to change it a little bit since I don't want to enforce a runtime dependency to PB classes.
That means, we can't use PB classes inside the generated builder's source code.
So we don't need to create our own copy of @GwtIncompatible.

Instead I would prefer to handle this case equivalent to the Optional case, where the user must configure which Optional class (the Guava one or that from Java 8) to use (done by the @GeneratePojoBuilder.withOptionalProperties=<class>).

I suggest to add a new element to the @GeneratePojoBuilder annotation, that allows to specify which concrete class to use for marking gwt-incompatible methods.

For example:

@GeneratePojoBuilder(markGwtIncompatible=com.google.gwt.core.shared.GwtIncompatible.class)
public class MyPojo {
 ...
}

The default value of this element would be Void.class, which means that those methods wouldn't get marked.

@elias-adam
Copy link

The @GwtIncompatible annotation is a compile time dependency, because the GWT compiler uses it when it translates the source code to JS. The @GeneratePojoBuilder annotation a compile the dependency itself, so there is no runtime dependency on @GwtIncompatible.

@mkarneim
Copy link
Owner

Alright, I will recheck this.

@dojcsak
Copy link
Contributor Author

dojcsak commented Nov 18, 2017

Hi mkarneim,

Thank you for your attention.

I agree with elias-adam, @GwtIncompatible is a compile-time dependency. It does not disturb “normal Java” usage. I tested it.

I copied @GwtIncompatible, because of GWT recommendation (in @GwtIncompatible Javadoc): “Since only the name of the annotation matters, Java libraries may use their own copy of this annotation class to avoid adding a compile-time dependency on GWT.”

I like your suggestion about parameterization and I can implement it, but because of GWT recommendation and @GwtIncompatible does not disturb previous usage I think it is the easiest way to add GWT compatibility.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 87.506% when pulling f0e6ea4 on dojcsak:add-support-for-gwt into 16ff2d1 on mkarneim:master.

@mkarneim mkarneim merged commit f0e6ea4 into mkarneim:master Nov 25, 2017
@mkarneim
Copy link
Owner

Alright, that looks good.
Since only the annotation's name is relevant, I moved it into our own package.

Thank you for your contribution!

@dojcsak
Copy link
Contributor Author

dojcsak commented Nov 25, 2017

Thank you for merged!

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

Successfully merging this pull request may close these issues.

None yet

4 participants