added optional configuration param 'translationsFile' that can be use… #149

treblereel · 2022-03-15T00:39:07Z

…d to pass XTB translation file to closure compiler

niloc132

Please add an integration test that confirms that this works, possibly building more than one locale if convenient.

build-caching/src/main/java/com/vertispan/j2cl/build/PropertyTrackingConfig.java

treblereel · 2022-03-15T06:04:26Z

hmm, looks like i have to revert my last commit (useFileConfig fix) coz it leads to:

[ERROR] Failed to execute goal com.vertispan.j2cl:j2cl-maven-plugin:0.20-SNAPSHOT:test (test-js) on project test: Failed to run tests: Bad artifact coordinates src/main/java/org/treblereel/test/MyTranslationBundle.xtb, expected format is <groupId>:<artifactId>[:<extension>[:<classifier>]]:<version> -> [Help 1]

and it doesn't matter where i put MyTranslationBundle.xtb

niloc132 · 2022-03-16T16:42:33Z

That error comes when a) the string is first used as a set of maven coords, and if that fails, b) the string is used as a path to pick out a local file. The idea is that you might want to point at an external artifact

        public File readFile() {
            String pathOrCoords = readString();
            if (pathOrCoords == null) {
                return null;
            }
            File f;
            // try to resolve as maven coords first
            try {
                f = getFileWithMavenCoords(pathOrCoords);
            } catch (ArtifactResolutionException e) {
                // handle as a file instead
                f = new File(pathOrCoords);
            }
            log.debug(getPath() + " => " + f.getAbsolutePath());
            return f;
        }

Probably you need to specify ${project.baseDir}/src/... on your file, so that it is clear what the real path to the file is? I could see it being ambiguous, such as a generated xtb file from some other source, like merged properties files?

niloc132 · 2022-03-20T12:38:33Z

...ven-plugin/src/it/translation/src/main/java/org/treblereel/test/TranslationService.native.js

+
+TranslationService.format = function () {
+    return MSG_SOMETHING_HAPPENED;
+}


semicolon, trailing newline

j2cl-maven-plugin/src/it/translation/src/main/webapp/WEB-INF/web.xml

niloc132 · 2022-03-20T12:49:21Z

j2cl-maven-plugin/src/it/translation/src/main/java/org/treblereel/test/holder.js

+ */
+goog.provide('holder');
+/** @define {string} */
+holder.value = goog.define('holder.value', 'unknown');


trailing newline, copyright notice not required (following current repo conventions anyway - i might be wrong about it, but i'm not going to object if this file is copied without permission).

niloc132 · 2022-03-20T12:49:48Z

j2cl-maven-plugin/src/it/translation/src/main/java/org/treblereel/test/TranslationService.java

+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */


not required to put this in (and you seem inconsistent about where you add these?)

niloc132 · 2022-03-20T12:51:38Z

j2cl-maven-plugin/src/it/translation/pom.xml

+    <module>pom_default.xml</module>
+    <module>pom_cz.xml</module>
+    <module>pom_fr.xml</module>
+  </modules>


consider one more project with three blocks, one for each test, each with its own block to specify the expected file, path

This is what I'm talking about, more than one execution in the same pom file, with different xtb file and local specified per execution - so that the user doesnt have to copy the whole pom file, just the execution.

build-caching/src/main/java/com/vertispan/j2cl/build/PropertyTrackingConfig.java

treblereel · 2022-03-21T21:45:46Z

@niloc132 thanks for youe reviews, now it works well with your #152 so i am going to rebase this branch with it once it ll be merged.

removed useless elemental2-dom
packages renamed to example.test
translation folder renamed to translationsFile
fixed typos
added more test cases

…d to pass XTB translation file to closure compiler

* removed useless elemental2-dom * packages renamed to example.test * translation folder renamed to translationsFile * fixed typos * added more test cases

niloc132 · 2022-03-22T02:53:32Z

build-caching/src/main/java/com/vertispan/j2cl/build/PropertyTrackingConfig.java

+    @Override
+    public Optional<File> getTranslationsFile() {
+        ConfigValueProvider.ConfigNode node = config.findNode("translationsFile");
+        if (node == null) {


your call here, but you could drop the null check if you just used ofNullable below ;)

old school, forever old school :)

niloc132 · 2022-03-22T02:59:32Z

j2cl-maven-plugin/src/it/translationsfile/tests/pom_cz.xml

+
+  <build>
+    <resources>
+      <resource>


i dont understand what purpose this serves? might be necessary if you put the translations file into a jar, but not requires in this project right?

niloc132 · 2022-04-07T03:50:55Z

j2cl-tasks/src/main/java/com/vertispan/j2cl/build/provided/JsZipBundleTask.java

@@ -54,6 +54,7 @@ public Task resolve(Project project, Config config) {
                    Collections.emptyList(),
                    Collections.emptyMap(),
                    Collections.emptyList(),
+                    config.getTranslationsFile(),


This won't work I think, it should error out actually - did you confirm that BUNDLE_JAR behaves when there is a translations file?

So two notes, first of all you can never make calls on config inside the lambda - it should error out (and i'm surprised it doesn't even with a non-empty translations file... worth looking into, that means nulls don't affect the cache), and second this is a BUNDLE so you should just give it Optional.empty()

niloc132 · 2022-04-07T03:53:24Z

j2cl-tasks/src/main/java/com/vertispan/j2cl/build/provided/ClosureBundleTask.java

@@ -96,6 +96,7 @@ public Task resolve(Project project, Config config) {
                    Collections.emptyList(),
                    Collections.emptyMap(),
                    Collections.emptyList(),//TODO actually pass these in when we can restrict and cache them sanely
+                    config.getTranslationsFile(),


as with JsZipBundleTask this shouldnt work, replace with Optional.empty()

niloc132 · 2022-04-07T03:53:45Z

j2cl-maven-plugin/src/main/java/com/vertispan/j2cl/mojo/TestMojo.java

@@ -244,6 +244,9 @@
    @Parameter(defaultValue = "false")
    protected boolean enableSourcemaps;

+    @Parameter


docs? can follow up in a later release, but it should be the same as BuildMojo

technically this property should be added to WatchMojo too, so that it is doc'd correctly, and can be set at the parent - see how languageOut, checkAssertions, rewritePolyfills are all declared but unused

are we going to run WatchMojo in advanced mode ? anyway, i ll add it

Yes, that's a thing, I'm not going to tell people they can't do it...

Reading it now, it looks like you did miss WatchMojo?

j2cl-maven-plugin/src/main/java/com/vertispan/j2cl/mojo/BuildMojo.java

niloc132 · 2022-04-19T03:29:41Z

j2cl-tasks/src/main/java/com/vertispan/j2cl/tools/Closure.java

@@ -112,6 +114,15 @@ public boolean compile(
            jscompArgs.add(extern);
        }

+        translationsFile.ifPresent(file -> {
+            if(compilationLevel.equals(CompilationLevel.ADVANCED_OPTIMIZATIONS)) {


stupid question perhaps, but are we sure this doesnt work with other modes?

...with that said, I dont think WHITESPACE_ONLY works at all right now, and I think SIMPLE is broken on windows

@niloc132 in this case we should create a check list of available modes and test PR againt them. I think, we can allow translationFiles to work with ADVANCED mode only.

Sounds good, defer it to another issue?

yes, i think so

niloc132 · 2022-04-24T18:29:33Z

I approved this, but please be sure to file that issue.

…ertispan#149) Closure-library offers some i18n features that closure-compiler will support at compile time by replacing strings based on the selected locale. This patch passes the required .xtb file to the closure compiler so that projects which use this feature can be specialized per-locale. With Vertispan#119, this could get more efficient, by optionally breaking up stage 3 closure compiler work into its own task.

niloc132 reviewed Mar 15, 2022

View reviewed changes

build-caching/src/main/java/com/vertispan/j2cl/build/PropertyTrackingConfig.java Outdated Show resolved Hide resolved

niloc132 reviewed Mar 20, 2022

View reviewed changes

treblereel added 5 commits March 21, 2022 16:30

added optional configuration param 'translationsFile' that can be use…

000c324

…d to pass XTB translation file to closure compiler

after review

9204fff

temporary revert node.readFile()

49dbae9

tests

4b475d3

* useFileConfig restored

6378930

* removed useless elemental2-dom * packages renamed to example.test * translation folder renamed to translationsFile * fixed typos * added more test cases

treblereel force-pushed the translationsFile branch from edb6ef3 to 6378930 Compare March 21, 2022 23:30

niloc132 reviewed Mar 22, 2022

View reviewed changes

treblereel added 3 commits March 24, 2022 18:50

cleanup

b4d154f

the fix

f81200f

added test with executions

41f78ed

niloc132 reviewed Apr 7, 2022

View reviewed changes

polishing

dcaf4cd

niloc132 reviewed Apr 19, 2022

View reviewed changes

treblereel added this to the 0.20 milestone Apr 19, 2022

treblereel added the enhancement New feature or request label Apr 19, 2022

treblereel self-assigned this Apr 19, 2022

niloc132 approved these changes Apr 24, 2022

View reviewed changes

niloc132 merged commit 4f092d0 into Vertispan:main May 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added optional configuration param 'translationsFile' that can be use… #149

added optional configuration param 'translationsFile' that can be use… #149

treblereel commented Mar 15, 2022

niloc132 left a comment

treblereel commented Mar 15, 2022

niloc132 commented Mar 16, 2022

niloc132 Mar 20, 2022

niloc132 Mar 20, 2022

niloc132 Mar 20, 2022

niloc132 Mar 20, 2022

niloc132 Mar 24, 2022

treblereel commented Mar 21, 2022

niloc132 Mar 22, 2022

treblereel Mar 22, 2022

niloc132 Mar 22, 2022

niloc132 Apr 7, 2022

niloc132 Apr 7, 2022

niloc132 Apr 7, 2022

niloc132 Apr 7, 2022

niloc132 Apr 7, 2022

treblereel Apr 7, 2022

niloc132 Apr 19, 2022

niloc132 Apr 19, 2022

niloc132 Apr 19, 2022

treblereel Apr 19, 2022

niloc132 Apr 19, 2022

treblereel Apr 19, 2022

niloc132 commented Apr 24, 2022

added optional configuration param 'translationsFile' that can be use… #149

added optional configuration param 'translationsFile' that can be use… #149

Conversation

treblereel commented Mar 15, 2022

niloc132 left a comment

Choose a reason for hiding this comment

treblereel commented Mar 15, 2022

niloc132 commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treblereel commented Mar 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niloc132 commented Apr 24, 2022