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

DevMode Blocks until changes are detected using WatchService #2

Open
wants to merge 2 commits into
base: app-building-tools
Choose a base branch
from

Conversation

Christopher-Chianelli
Copy link

@niloc132 Since I cannot make a private repo on GitHub, and https://github.com/mbarkley/j2cl-maven-plugin is a completely different app from this, I am pushing my contributions to a different branch of this fork. Few questions:

  1. I cannot run DevMode.java properly since the JRE is not finding some classes (namely com/google/j2cl/common/Problems and CompilationLevel.BUNDLE). Are you using a different fork of closure than the one at https://github.com/google/closure-library and https://github.com/google/closure-compiler? I did verify the file collection process worked (could only do 1 run though since it then crashed)

  2. What formatter do you use (so our commits don't contain a bunch of reformatting already formatted code to another format)?

  3. What are the java9/osx issues you mentioned for FileService?

  4. Will the main maven plugin be developed here or at https://github.com/mbarkley/j2cl-maven-plugin? If it will be developed here, I will port more features that my plugin has to the main app here.

-Created a new class, ResourceListener, to collect File changes
detected by the WatchService
-Previously, DevMode was checking lastModified times every
0.1 seconds
@korzha
Copy link

korzha commented Nov 23, 2017

I can answer some questions here.

  1. Closure compiler is modified and deployed to Colin's artifactory. Just ask him and he grants you access

  2. Gwt projects use google java code style http://www.gwtproject.org/makinggwtbetter.html#codestyle

  3. ?

  4. I suggest to develop maven plugin at https://github.com/mbarkley/j2cl-maven-plugin
    I am going to merge your pull request there (it's awesome) and port Colin's DevMode and mine changes to this plugin repo (i hope I can push it this weekend).

Update: I see that Colin doesnt use google java code style here. Sorry for confusion.

@Christopher-Chianelli
Copy link
Author

@nordligulv Thanks for the quick reply! To be honest, I waited way too long to push the commits in the PR in https://github.com/mbarkley/j2cl-maven-plugin, which probably should have been done as several smaller PR's (or even just pushed straight to master if the commit was small enough).

@korzha
Copy link

korzha commented Nov 23, 2017

Sorry for the long feedback. I started late and it took me a while to get in how things work. Promise to review your changes much quicker now if you have something!

@tbroyer
Copy link

tbroyer commented Nov 23, 2017

Gwt projects use google java code style http://www.gwtproject.org/makinggwtbetter.html#codestyle

This is actually GWT code style; Google code style is a bit different: https://google.github.io/styleguide/javaguide.html (e.g. import order have changed a few months ago)

I, for one, use Google Java Format so I don't have to care about my writing style or my own preferences.

@niloc132
Copy link
Member

  1. You can fork this (or the upstream one) and add it as a remote to your local branch? takes a but more messing around, but might be an easier way to swap code.

  2. You (@Christopher-Chianelli, others may need to ask me) should already have access to our maven repo to pull com.vertispan.javascript:closure-compiler-unshaded (strongly advise using unshaded so you can exclude dependencies without going insane - guava and jsinterop-annotations both are out of date):

    <dependency>
      <groupId>com.vertispan.javascript</groupId>
      <!--<groupId>com.google.javascript</groupId>-->
      <artifactId>closure-compiler-unshaded</artifactId>
      <version>1.0-SNAPSHOT</version>
      <exclusions>
        <exclusion>
          <groupId>com.google.jsinterop</groupId>
          <artifactId>jsinterop-annotations</artifactId>
        </exclusion>
        <exclusion>
          <groupId>com.google.guava</groupId>
          <artifactId>guava</artifactId>
        </exclusion>
      </exclusions>
    </dependency>

Otherwise, this is master at https://github.com/vertispan/closure-compiler, and you can build yourself.

  1. I'll switch to any of the above, someone who cares enough should lay down the law. The app-building-tools is 100% strawman code, and should be used only as a guide of where I think the process might take us, should not be taken as "correct" or even "worth trying to salvage". Can you point out what you used for the formatting? I'll apply the same thing and rebase your changes afterward. My two cents for doing this in the future: one commit just for formatting changes (which can be merged pretty much without review), then append additional commits with substantive changes.

  2. https://docs.gradle.org/current/userguide/continuous_build.html#sec:continuous_build_limitations_jdk9 is where I heard about issues. I'm on OS X, but havent pulled down java9 except in a docker instance yet.

  3. I'd defer to @tbroyer here, but it sounds like a second plugin could be preferred. Just not sure how they will interact in library projects that happen to be both gwt2 and gwt3 compatible, if they each provide their own packaging...

@tbroyer
Copy link

tbroyer commented Nov 24, 2017

  1. Fwiw, here's the Maven plugin for Google Java Format: https://github.com/coveo/fmt-maven-plugin (please do not bind the format goal to the lifecycle, only the check one; and use mvn fmt:format to reformat code; there are also plugins for Eclipse and IntelliJ IDEA)

  2. This is actually a limitation of Gradle only, not the JDK: [Modular Gradle] Start Gradle with the necessary Java modules gradle/gradle#722 (comment) (this issue comment is more about jigsaw modules, but the links at the end explain how to make it work)

  3. I don't have access to j2cl-maven-plugin yet, so can't really comment (yet)
    (btw, @niloc132, could you give me access to your Maven repo too? I don't really need it but would be great to have some shared baseline)

@korzha
Copy link

korzha commented Nov 24, 2017

  1. Thanks for formatting plugin @tbroyer
    I sent a pr

  2. I guess @mbarkley could add you to the repo.

niloc132 pushed a commit that referenced this pull request Jul 26, 2022
…sion.

The CL also
 - introduces WasmOpaque (so we can pass char[] to JS back and forth)
 - fixes various scenarios that ImplementArraysAsClasses wasn't handling.

I also tried alternative to WasmOpaque: using "data" for all non-primitive types in extern method signatures. That seemed cleaner at first but didn't go well for 2 reasons:
1. Even it is ok at params, returns are not assignable so got messy around typing.
2. We had also existing externs that affected by the mess is #2.

Having an explicit WasmOpaque type ended up being cleaner which enforced both the necessary cast and the unnecessary casts around j.l.Object (which we needed to remove because they were incorrect because native array is not assignable to j.l.Object).

PiperOrigin-RevId: 455572885
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