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

Implemented: Use NPM with gradle to get external JS dependencies (OFBIZ-11960) #230

Merged
merged 12 commits into from Oct 12, 2021

Conversation

adityasharma7
Copy link
Member

  1. Added gradle-node-plugin Gradle plugin for integrating NodeJS in your build (https://github.com/node-gradle/gradle-node-plugin)
  2. Created NPM package.json with JS dependencies in webapp based upon https://docs.npmjs.com/files/package.json
  3. Added license information in package.json based upon SPDX License List (https://spdx.org/licenses/) as supported by NPM (https://docs.npmjs.com/files/package.json#license)
  4. Added package-lock.json as suggested in NPM documentation (https://github.com/npm/cli/blob/release-6.14.7/docs/content/configuring-npm/package-lock-json.md#description)
  5. Added jQuery and jQuery Migrate using NPM and used it throughout

…IZ-11960)

1. Added gradle-node-plugin Gradle plugin for integrating NodeJS in your build (https://github.com/node-gradle/gradle-node-plugin)
2. Created NPM package.json with JS dependencies in webapp based upon https://docs.npmjs.com/files/package.json
3. Added license information in package.json based upon SPDX License List (https://spdx.org/licenses/) as supported by NPM (https://docs.npmjs.com/files/package.json#license)
4. Added package-lock.json as suggested in NPM documentation (https://github.com/npm/cli/blob/release-6.14.7/docs/content/configuring-npm/package-lock-json.md#description)
5. Added jQuery and jQuery Migrate using NPM and used it throughout
@JacquesLeRoux
Copy link
Contributor

Hi Suraj,

You need to merge trunk HEAD, conflicts here and locally:

C:\projectsASF\Git\ofbiz-framework>git apply 230.patch
error: patch failed: framework/common/src/main/java/org/apache/ofbiz/common/JsLanguageFilesMapping.java:31
error: framework/common/src/main/java/org/apache/ofbiz/common/JsLanguageFilesMapping.java: patch does not apply
error: patch failed: themes/common-theme/template/JsLanguageFilesMapping.ftl:74
error: themes/common-theme/template/JsLanguageFilesMapping.ftl: patch does not apply

Else after review sounds good to me

TIA

@adityasharma7
Copy link
Member Author

Hi Jacques,

It seems with Suraj you are referring to me here.

Though there is some work left at my part:

  1. Check if there are more libraries that can be used through NPM. I did it for JQuery, JQuery migrate, JQuery browser, jQuery validate and I faced some issues with JQuery UI library so we will continue to use the downloaded one.
  2. Remove migrated JS distribution and updated the ReadMe with steps to download using NPM

I will try to get it done in a week

You can certainly test it with the current version so that if there are some inputs we will address them now only. You will have to follow these steps:

  1. Execute ./gradlew npmInstall after which the libraries listed as dependencies themes/common-theme/webapp/common/js/package.json gets downloaded at themes/common-theme/webapp/common/js/node_modules/
  2. You can locate the folder for each dependency in node_modules containing the package that we usually need to manually download.
  3. You can refer Network tab to check if the files are loaded from the node_modules.

@sonarcloud
Copy link

sonarcloud bot commented Aug 28, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
4.2% 4.2% Duplication

@JacquesLeRoux
Copy link
Contributor

THanks Suraj,

It's ok here, but can't apply locally:

C:\projectsASF\Git\ofbiz-framework>git pull
Already up to date.

C:\projectsASF\Git\ofbiz-framework>git apply 230.patch
error: patch failed: framework/common/src/main/java/org/apache/ofbiz/common/JsLanguageFilesMapping.java:31
error: framework/common/src/main/java/org/apache/ofbiz/common/JsLanguageFilesMapping.java: patch does not apply
error: patch failed: themes/common-theme/template/JsLanguageFilesMapping.ftl:74
error: themes/common-theme/template/JsLanguageFilesMapping.ftl: patch does not apply

C:\projectsASF\Git\ofbiz-framework>

@JacquesLeRoux
Copy link
Contributor

Oops really sorry (again) Aditya,

I was working on other things at the same time and got confused 😊. Moreover we crossed online.

Thanks for your explanation. I'll wait a week rather ;)

@adityasharma7
Copy link
Member Author

No issues Jacques :)
I will check once done the issue with applying the patch.

@surajkhurana
Copy link
Contributor

:)

@JacquesLeRoux
Copy link
Contributor

Hi Aditya,

I just checked and we are still in the same situation, TIA

@JacquesLeRoux
Copy link
Contributor

JacquesLeRoux commented Nov 8, 2020

Hi Aditya,

Just noticed that we are no longer using date.format in trunk, ie this line:

It was removed by https://issues.apache.org/jira/browse/OFBIZ-12040
I'll remove it from the LICENSE file

@adityasharma7 adityasharma7 changed the title WIP: Implemented: Use NPM with gradle to get external JS dependencies (OFBIZ-11960) Implemented: Use NPM with gradle to get external JS dependencies (OFBIZ-11960) May 17, 2021
@adityasharma7
Copy link
Member Author

Hi @JacquesLeRoux,
This is done now. Initially, I have added only jQuery, jQuery browser, and Validate plugin through npm. As these are the libraries required for the front end to work I have kept the npmInstall task part of the Java build, so it executes on each tasks.

Steps to verify:

  1. Fetch the branch on your local
  2. Start the server
    You will notice packages being downloaded with npmInstall task

image

You will find the node_modules under the js folder

image

@sonarcloud
Copy link

sonarcloud bot commented May 17, 2021

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
3.7% 3.7% Duplication

@JacquesLeRoux
Copy link
Contributor

Hi Aditya,

I tried, got:

C:\projectsASF\Git\Jacques\ofbiz-framework>gradlew clean build
Starting a Gradle Daemon, 3 busy Daemons could not be reused, use --status for details

> Configure project :
You are using one or more deprecated Asciidoctor Gradle plugin features. To help with migration run with --warning-mode=all.

> Task :clean UP-TO-DATE

> Task :compileJava
Z:\Gradle\caches\modules-2\files-2.1\org.apache.tomcat\tomcat-el-api\9.0.43\ce73ae606e6bee7df54e4a7701e51886e793d3ac\tomcat-el-api-9.0.43.jar(javax/el/ExpressionFactory.class): warning: Cannot find annotation
 method 'value()' in type 'ServiceConsumer': class file for aQute.bnd.annotation.spi.ServiceConsumer not found
1 warning

> Task :compileGroovy
> Task :nodeSetup
> Task :npmSetup SKIPPED
> Task :processResources
> Task :classes
> Task :compileGroovyScriptsJava NO-SOURCE
> Task :compileTestJava

> Task :npmInstall
added 4 packages from 3 contributors and audited 4 packages in 2.393s
found 1 moderate severity vulnerability
  run `npm audit fix` to fix them, or `npm audit` for details

> Task :jar
> Task :startScripts
> Task :distTar
> Task :distZip
> Task :assemble
> Task :compileGroovyScriptsGroovy SKIPPED
> Task :processGroovyScriptsResources NO-SOURCE
> Task :groovyScriptsClasses UP-TO-DATE
> Task :checkstyleGroovyScripts NO-SOURCE

> Task :checkstyleMain
Checkstyle rule violations were found. See the report at: file:///C:/projectsASF/Git/Jacques/ofbiz-framework/build/reports/checkstyle/main.html
Checkstyle files with violations: 57
Checkstyle violations by severity: [error:115]


> Task :compileTestGroovy
> Task :processTestResources
> Task :testClasses
> Task :checkstyleTest
> Task :test
> Task :check
> Task :build

BUILD SUCCESSFUL in 5m 14s
16 actionable tasks: 15 executed, 1 up-to-date
C:\projectsASF\Git\Jacques\ofbiz-framework>

So it works for me and I indeed see very good progress with this way.
I don't think we need to worry about

SonarCloud Code Analysis Failing after 15m — Quality Gate failed
Looking briefly at the cascading links, I guess it's related to already existing Js code we have.

So +1 with me for merging

@danwatford
Copy link
Contributor

Hello,

I wanted to look at the details of the code duplication error that SonarCloud has raised, but it looks like the report has expired since it was last run in May.]

In GitHub's Checks tab for this PR, I can click 'Re-run' to request that the SonarCloud checks are run again, but they do not appear to run.

Does anyone know how to get SonarCloud to re-check this PR without committing another change to Aditya's branch?

@JacquesLeRoux
Copy link
Contributor

Hi Daniel, I think you should not worry about that. I have recently changed the SonarCloud setting to avoid false positive, most were related to js libs and we don't have to worry about that.

I thought propose to commit a fake change in order to verify my assertion ;)

@danwatford
Copy link
Contributor

It looks like the issues raised by SonarCloud have been addressed on trunk.

@sonarcloud
Copy link

sonarcloud bot commented Oct 10, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@danwatford danwatford merged commit 4bd30ce into apache:trunk Oct 12, 2021
@adityasharma7 adityasharma7 deleted the OFBIZ-11960 branch January 3, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants