Skip to content

/ ofbiz-framework

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

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

Open
wants to merge 5 commits into
base: trunk
from
Open

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

wants to merge 5 commits into from

Conversation

@adityasharma7
Copy link
Member

adityasharma7 commented Aug 22, 2020

  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
@adityasharma7
Implemented: Use NPM with gradle to get external JS dependencies (OFB…
Loading status checks…
2fea4af 
…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
@adityasharma7
Added: jQuery Browser using NPM and used it throughout (OFBIZ-11960)
e28ff88
@adityasharma7
Added: jQuery validation using NPM and used it throughout (OFBIZ-11960)
0d10d61
@adityasharma7
Improved: added node_modules to gitignore file (OFBIZ-11960)
Loading status checks…
58faf6f
@JacquesLeRoux
Copy link
Contributor

JacquesLeRoux commented Aug 28, 2020

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
Merge branch 'trunk' of https://github.com/apache/ofbiz-framework int…
Loading status checks…
03d2e70 
…o OFBIZ-11960
@adityasharma7
Copy link
Member Author

adityasharma7 commented Aug 28, 2020

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

JacquesLeRoux commented Aug 28, 2020

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

JacquesLeRoux commented Aug 28, 2020

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 ;)
1 similar comment
@JacquesLeRoux
Copy link
Contributor

JacquesLeRoux commented Aug 28, 2020

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

adityasharma7 commented Aug 28, 2020

No issues Jacques :)
I will check once done the issue with applying the patch.
@surajkhurana
Copy link
Contributor

surajkhurana commented Aug 28, 2020

:)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@adityasharma7 @JacquesLeRoux @surajkhurana
You can’t perform that action at this time.