Implement support for WebJars and LESS as assets pipeline #132

fushar · 2020-03-13T18:52:54Z

Full support for WebJars as described here: https://www.playframework.com/documentation/2.8.x/AssetsOverview#WebJars
- Add plugin org.gradle.playframework-webjars
- Add webJar configuration for adding dependency to WebJar libraries, e.g.
```
  dependencies {
      webJar 'org.webjars:requirejs:2.3.6'
  }
```
- Extracted jars are under lib/ in the assets jar
- Implementation: https://github.com/webjars/webjars-locator-core
Full support for LESS as described here:
https://www.playframework.com/documentation/2.8.x/AssetsLess
- Add plugin org.gradle.playframework-less
- Add less source set
- Can import partials (_*.less)
- Can import sources from lib/ generated by WebJars above
- Implementation: https://github.com/SomMeri/less4j
Include the following to the runPlay's assets dirs so that the compiled/generated assets can be served during development mode:
- WebJars outputs
- Compiled LESS -> CSS outputs
- Minified JavaScript (https://github.com/gradle/playframework/pull/132/files#diff-e7ebc2272b935d9432ec15917a15b01eR52)
Add integration tests
Add asset pipelines docs

JLLeitschuh · 2020-03-30T19:52:03Z

This is an impressive bit of work. Thanks for your contribution!

JLLeitschuh · 2020-06-15T20:14:34Z

Looks like the docs test are failing, which is abnormal as they aren't failing on master. Could you take a look and let me know why?

fushar · 2020-06-16T14:39:29Z

@JLLeitschuh Oops. I forgot to add declare the new less plugin in a build.gradle in docs. I've pushed the fix. Here is the diff: https://github.com/gradle/playframework/compare/ecb94d550f6c50a8cfe39c4aa9761efc46092e85..cba5cb53f80b3c4a01b81268a38e03fb637c02f9

But now somehow it does not trigger Travis build now 😅 what is going on?

fushar · 2020-06-17T11:00:42Z

@JLLeitschuh the CI test has run now. Doc test is green :)

JLLeitschuh · 2020-06-18T13:44:00Z

Before merging this, I'd like to see master fixed. See #136

fushar · 2020-09-27T09:07:21Z

@JLLeitschuh I've rebased to latest master, now everything is green, could you take a look again?

JLLeitschuh

Looks good so far! A few changes I'd appreciate. I'm trying to get someone else to take a look as well.

src/integTest/groovy/org/gradle/playframework/tasks/AbstractAssetsTaskIntegrationTest.groovy

JLLeitschuh · 2020-10-05T19:03:47Z

src/main/java/org/gradle/playframework/plugins/PlayWebJarsPlugin.java

+        project.getTasks().named(PLAY_RUN_TASK_NAME, PlayRun.class, task -> {
+            task.getAssetsDirs().from(webJarsExtractTaskProvider.get().getOutputDirectory());
+        });
+        project.getTasks().matching(task -> task.getName().equals(LESS_COMPILE_TASK_NAME)).forEach(task -> {


This forces the realization of all tasks. Can you use a type constraint instead of a name constraint?

Ah, just realized this. Changed this to:

Optional.ofNullable(project.getTasks().findByName(LESS_COMPILE_TASK_NAME)).ifPresent(task -> {

Can't find any method which accepts type constraint but does not fail if the task is not found :(

Asking internally about this. I think that findByName is still forcing some task configuration & realization?

To be honest, I'm not really sure about this. The thing is I cannot use .named() because it will throw if the less plugin is not applied. Most of the methods I found is doing the similar thing (throwing when task is not found).

Could you advise the correct method? I could not find anything by browsing the Task javadoc / documentation :(

After asking internally, the answer I got was tasks.matching { ... }.configureEach { ... } This may be what you already had (sorry). But supposedly, this is what you want to use here.

I see. No worries, I added it back!

src/main/java/org/gradle/playframework/tasks/WebJarsExtract.java

src/main/java/org/gradle/playframework/tools/internal/less/Less4jCompiler.java

src/main/java/org/gradle/playframework/tools/internal/webjars/WebJarsExtractor.java

fushar · 2020-10-13T03:00:05Z

@JLLeitschuh Thanks for reviewing; I have addressed/answered all your comments.

JLLeitschuh · 2020-10-14T19:30:36Z

src/main/java/org/gradle/playframework/sourcesets/LessSourceSet.java

+ * }
+ * </pre>
+ */
+public interface LessSourceSet {


Out of curiosity, does this become accessible via the Kotlin DSL as well?

Honestly I am not using Kotlin DSL so I cannot verify. But I believe it should because it follows exactly the same pattern as the other source sets in the existing codebase.

JLLeitschuh · 2020-10-14T19:32:01Z

src/main/java/org/gradle/playframework/tasks/LessCompile.java

+        return lessCompilerClasspath;
+    }
+
+    public List<File> getIncludePaths() {


Is this an input or an output?

Ah, interesting, this could be @InputFiles. Added!

JLLeitschuh · 2020-10-14T19:32:20Z

src/main/java/org/gradle/playframework/tasks/LessCompile.java

+        return outputDirectory;
+    }
+
+    @Classpath


Does this infer an input/output or does it need to be explicit?

Actually I was just following existing JavaScriptMinify task here: https://github.com/gradle/playframework/blob/master/src/main/java/org/gradle/playframework/tasks/JavaScriptMinify.java#L67

But it seems that according to the docs here: https://docs.gradle.org/current/userguide/more_about_tasks.html I should also add @InputFiles to be compatible with some older Gradle versions, which I do not actually use. Should I still add it?

JLLeitschuh

I know I continue to be nitpicky. This is just a large change. This should hopefully be the last review pass I need to take on this.

JLLeitschuh · 2020-10-14T19:35:19Z

@fushar are you willing to own this change a bit after it is merged and help manage issues/bugs that are associated with it moving forward?

fushar · 2020-10-15T15:36:55Z

@fushar are you willing to own this change a bit after it is merged and help manage issues/bugs that are associated with it moving forward?

Sure, I am still maintaining a project which heavily relies on these plugins, so I should be able to help on that.

JLLeitschuh · 2020-10-15T20:44:13Z

Thanks @fushar! We just did a release, we're going to try to get this into the next one, hopefully later today, or tomorrow.

Signed-off-by: Ashar Fuadi <[email protected]>

JLLeitschuh · 2020-11-02T19:31:56Z

Hey @fushar,

Really sorry. I went on vacation the past two weeks and thought that someone would follow up here.

I had a discussion with @big-guy about this. We were wondering why the webjar component needed to be a part of this plugin and couldn't be a wholly-separate community maintained plugin? It doesn't seem like it's play specific.

fushar · 2020-11-12T14:11:25Z

Hi @JLLeitschuh sorry for late reply; was busy with work.

I was just trying to get feature parity with the original SBT plugin counterpart. As per the original docs, WebJars is a built-in feature, thus this PR.

I actually had tried writing 2 separate plugins for WebJars and LESS, based on the deprecated Gradle Software Model. However, the LESS plugins actually depends on WebJars output. That's also another reason why I put those two together in the same codebase.

What do you think? Should I still somehow separate those two?

JLLeitschuh · 2020-11-19T14:51:08Z

@fushar I think that your rationale for why you're including it makes sense. Let me take another pass quickly.

JLLeitschuh

I understand @big-guy's hesitation to add the web jar functionality to this plugin, but I think that it's a worthy addition. I'm giving this my LGTM approval.

I'm meeting with @big-guy later today, I'll make sure I run this by him one last time during that meeting

fushar · 2020-11-19T15:15:37Z

Thanks @JLLeitschuh, appreciate it 😄

JLLeitschuh · 2020-11-19T19:58:39Z

src/main/java/org/gradle/playframework/plugins/PlayWebJarsPlugin.java

+        project.getTasks().matching(task -> task.getName().equals(LESS_COMPILE_TASK_NAME)).configureEach(task -> {
+            LessCompile lessCompileTask = (LessCompile) task;
+            lessCompileTask.dependsOn(webJarsExtractTaskProvider);
+            lessCompileTask.getIncludePaths().add(webJarsExtractTaskProvider.get().getOutputDirectory().get().getAsFile());
+        });


Instead of doing this, can you use named inside of a check to make sure that the less compile plugin is applied?

JLLeitschuh · 2020-11-19T23:21:12Z

Hey @fushar,

So, after a long conversation with @big-guy we decided that we unfortunately don't want to include these plugins to this repo. However, we are more than willing to add tests to this repository to verify that an external plugin stays compatible with a plugin that adds this functionality and would be happy to link to an external plugin from our documentation.

As a part of this, this will give you the freedom to use some of the newer APIs and will keep that external plugin from being constrained to the same Gradle version support as this one.

Of note, some of the patterns and paradigms you mimicked from other code in this plugin was a carryover from the original port and extraction of this code from Gradle/Gradle. That extraction maintained some of the less-than-optimal patterns of the Gradle Software Model that is now deprecated. As a standalone plugin you can more easily adopt some of the newer provider based APIs that automatically track task dependencies without requiring the use of dependsOn.

We want to thank you for your contribution, and we hope you will contribute more to this plugin in the future.

fushar · 2020-11-20T13:34:38Z

Hi @JLLeitschuh & @big-guy,

Ah... I see. Understandable. Though most probably I don't really have the bandwidth in the near future to pick this up (make separate plugins, re-testing, rewriting the docs changes etc). So for now I'll close this PR and live with my own fork for the time being. Hopefully i can revisit this some time next year.

Thanks for your time reviewing this PR! 🙂

fushar force-pushed the assets-less-webjars branch from d840c1d to ecb94d5 Compare March 13, 2020 18:53

JLLeitschuh requested a review from big-guy March 30, 2020 19:51

fushar force-pushed the assets-less-webjars branch from ecb94d5 to cba5cb5 Compare June 16, 2020 14:30

fushar force-pushed the assets-less-webjars branch from cba5cb5 to 554c8b4 Compare September 19, 2020 08:02

fushar force-pushed the assets-less-webjars branch from 554c8b4 to fe8c126 Compare September 27, 2020 06:54

cstroe approved these changes Sep 30, 2020

View reviewed changes

JLLeitschuh suggested changes Oct 5, 2020

View reviewed changes

fushar force-pushed the assets-less-webjars branch from fe8c126 to c51cf14 Compare October 12, 2020 23:05

JLLeitschuh reviewed Oct 14, 2020

View reviewed changes

JLLeitschuh suggested changes Oct 14, 2020

View reviewed changes

fushar force-pushed the assets-less-webjars branch from c51cf14 to 0bf3a23 Compare October 15, 2020 15:34

fushar force-pushed the assets-less-webjars branch 5 times, most recently from 00b3a64 to 2b905a2 Compare October 17, 2020 03:02

Implement support for WebJars and LESS assets pipeline

2edfc45

Signed-off-by: Ashar Fuadi <[email protected]>

fushar force-pushed the assets-less-webjars branch from 2b905a2 to 2edfc45 Compare October 17, 2020 03:05

JLLeitschuh approved these changes Nov 19, 2020

View reviewed changes

JLLeitschuh reviewed Nov 19, 2020

View reviewed changes

fushar closed this Nov 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for WebJars and LESS as assets pipeline #132

Implement support for WebJars and LESS as assets pipeline #132

fushar commented Mar 13, 2020 •

edited

Loading

JLLeitschuh commented Mar 30, 2020

JLLeitschuh commented Jun 15, 2020

fushar commented Jun 16, 2020

fushar commented Jun 17, 2020

JLLeitschuh commented Jun 18, 2020

fushar commented Sep 27, 2020

JLLeitschuh left a comment

JLLeitschuh Oct 5, 2020

fushar Oct 13, 2020

JLLeitschuh Oct 13, 2020

fushar Oct 14, 2020

JLLeitschuh Oct 14, 2020

fushar Oct 15, 2020

fushar commented Oct 13, 2020

JLLeitschuh Oct 14, 2020

fushar Oct 15, 2020

JLLeitschuh Oct 14, 2020

fushar Oct 15, 2020

JLLeitschuh Oct 14, 2020

fushar Oct 15, 2020

JLLeitschuh left a comment

JLLeitschuh commented Oct 14, 2020

fushar commented Oct 15, 2020

JLLeitschuh commented Oct 15, 2020

JLLeitschuh commented Nov 2, 2020

fushar commented Nov 12, 2020

JLLeitschuh commented Nov 19, 2020

JLLeitschuh left a comment

fushar commented Nov 19, 2020

JLLeitschuh Nov 19, 2020

JLLeitschuh commented Nov 19, 2020

fushar commented Nov 20, 2020

Implement support for WebJars and LESS as assets pipeline #132

Implement support for WebJars and LESS as assets pipeline #132

Conversation

fushar commented Mar 13, 2020 • edited Loading

JLLeitschuh commented Mar 30, 2020

JLLeitschuh commented Jun 15, 2020

fushar commented Jun 16, 2020

fushar commented Jun 17, 2020

JLLeitschuh commented Jun 18, 2020

fushar commented Sep 27, 2020

JLLeitschuh left a 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

fushar commented Oct 13, 2020

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

JLLeitschuh left a comment

Choose a reason for hiding this comment

JLLeitschuh commented Oct 14, 2020

fushar commented Oct 15, 2020

JLLeitschuh commented Oct 15, 2020

JLLeitschuh commented Nov 2, 2020

fushar commented Nov 12, 2020

JLLeitschuh commented Nov 19, 2020

JLLeitschuh left a comment

Choose a reason for hiding this comment

fushar commented Nov 19, 2020

Choose a reason for hiding this comment

JLLeitschuh commented Nov 19, 2020

fushar commented Nov 20, 2020

fushar commented Mar 13, 2020 •

edited

Loading