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

Use npm for js depdencies, bundle with webpack #13156

Merged
merged 14 commits into from
Jan 9, 2019
Merged

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Dec 19, 2018

Fixes #13071

Checklist:

  • "autosize": "4.0.2",
  • "backbone": "1.2.3",
  • "base64": "0.3.0",
  • "blueimp-md5": "2.7.0",
  • "bootstrap": "3.3.7",
  • "clipboard": "1.7.1",
  • "davclient.js": "https://github.com/evert/davclient.js.git#0.1.2", -> not available on npm installed via github
  • "handlebars": "4.0.5",
  • "jcrop": "0.9.12", -> no compatible version on npm, newer one does not work
  • "jquery": "2.1.4",
  • "jquery-migrate": "1.4.0", -> breaks somehow doesn't anymore
  • "jquery-ui": "1.12.1",
  • "jsTimezoneDetect": "1.0.6",
  • "marked": "0.3.6",
  • "moment": "2.18.1",
  • "select2": "3.4.8", -> only newer, not working versions on npm found one
  • "snapjs": "2.0.0-rc1",
  • "strengthify": "0.5.5", -> no npm package NPM package? strengthify#17 made it one Make this a npm package strengthify#25
  • "underscore": "1.8.3",
  • "zxcvbn": "4.4.2",
  • "es6-shim": "^0.35.4"

(Based on https://github.com/nextcloud/server/blob/ad9ece4e4839297e50d34bb50481a78407f6b835/bower.json)

@rullzer
Copy link
Member

rullzer commented Dec 19, 2018

zxcvbn

I guess we'd want to leave that one separate. As it is huge.

@ChristophWurst
Copy link
Member Author

I guess we'd want to leave that one separate. As it is huge.

Hugely unmaintained? https://github.com/dropbox/zxcvbn/commits/master

@ChristophWurst
Copy link
Member Author

Found https://www.npmjs.com/package/asdfgh. That might be a nice replacement one day.

@rullzer
Copy link
Member

rullzer commented Dec 19, 2018

👍 would probably make most sense if strengthify uses that directly in a package etc.

@ChristophWurst
Copy link
Member Author

Impressive!

$ du core/vendor/core.js 
988     core/vendor/core.js
$ du core/js/dist/main.js
972     core/js/dist/main.js

… and that even includes more scripts and styles 🚀

@ChristophWurst
Copy link
Member Author

* zxcvbn

This one lib needs special treatment. It's not used directly but loaded dynamically (as @rullzer commted above due to its size). Hence I would propose to leave it vendored, open an enhancement ticket and fix this later on. This PR is already huge and I don't want to start touching any scripts.

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 20, 2018
@ChristophWurst
Copy link
Member Author

cc @nextcloud/javascript for review/testing :)

package.json Show resolved Hide resolved
@rullzer
Copy link
Member

rullzer commented Dec 20, 2018

🚀!

So I guess at certain places we can remove additional scripts loaded etc.
But we can followup on that.

So far I did not notice anything doing 💥 yet!

@ChristophWurst

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@juliusknorr

This comment has been minimized.

ChristophWurst and others added 7 commits January 9, 2019 15:02
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
…effects

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member

Rebased and pushed a fix for the select2 result list. Really nice @ChristophWurst 👍

@ChristophWurst

This comment has been minimized.

"modules": false,
"targets": {
"browsers": [
"last 2 versions",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just reminded me https://jamie.build/last-2-versions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah … we can improve that later 😉

@MorrisJobke MorrisJobke added this to the Nextcloud 16 milestone Jan 9, 2019
@ChristophWurst

This comment has been minimized.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍 Tested and works 👍

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Jan 9, 2019
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Copy link
Member Author

So https://drone.nextcloud.com/nextcloud/server/14388/248 still fails with

Fixed. This was apparently an issue with 3.4.5 (the latest 3.4 version from npm). Updating to 3.4.8 (the version installed via bower) via git fixed the issue 🚀

@MorrisJobke
Copy link
Member

Fixed. This was apparently an issue with 3.4.5 (the latest 3.4 version from npm). Updating to 3.4.8 (the version installed via bower) via git fixed the issue 🚀

Let's see if https://drone.nextcloud.com/nextcloud/server/14389/248 is working.

@MorrisJobke
Copy link
Member

Let's see if https://drone.nextcloud.com/nextcloud/server/14389/248 is working.

Yes it does \o/

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Jan 9, 2019
@rullzer rullzer merged commit 011aab5 into master Jan 9, 2019
@rullzer rullzer deleted the enhancement/npmize branch January 9, 2019 19:16
@coliff
Copy link
Contributor

coliff commented Mar 5, 2019

Hi @ChristophWurst - I saw above you included "bootstrap": "3.3.7", - this version has a couple of XSS vulnerabilities and is not recommended to use. Can you please update that to v3.4.1?
REF:
https://blog.getbootstrap.com/2019/02/13/bootstrap-4-3-1-and-3-4-1/
https://snyk.io/vuln/npm:bootstrap

@ChristophWurst
Copy link
Member Author

Yep, we are aware of it and working on integrating the new version: #14352

We are on 3.4.1 meanwhile:

"bootstrap": "^3.4.1",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants