Move uglify invocation into npm scripts and remove grunt #628

nornagon · 2016-09-22T16:08:00Z

Unfortunately the uglifyjs command line ends up being a little unwieldy, but that's a small price to pay for dropping grunt!

If the 'watch' functionality is useful to folks, I can add one of the options from https://www.keithcirkel.co.uk/how-to-use-npm-as-a-build-tool/#watch

xPaw

🎉

nornagon · 2016-09-22T16:18:07Z

Oh dear, windows' lack of globbing strikes again. I'll figure something out.

nornagon · 2016-09-23T04:00:47Z

Some notes:

Windows doesn't support globbing (client/js/libs/*.js doesn't expand to the list of js files in windows shell)
Most nodejs libraries seem to solve this by adding a dependency on glob (which npm itself depends on), and parsing their cmd-line with it
UglifyJS is a unique snowflake and implements their own version of globbing, which will only glob-expand once per argument, so foo/*/*.js doesn't work.
Ultimately I think these libs/ should be either moved to dependencies installed via npm, included in the main bundle by import, or removed altogether.
Uglify can then be part of the webpack build process, and this npm script will go away.

So I think it's OK to explicitly list out the subdirectories for now.

astorije · 2016-09-23T05:26:15Z

package.json

    "build:font-awesome": "node scripts/build-fontawesome.js",
-    "build:grunt": "grunt",
+    "build:libs": "uglifyjs client/js/libs/*.js client/js/libs/jquery/*.js client/js/libs/handlebars/*.js -o client/js/libs.min.js --source-map client/js/libs.min.js.map --source-map-url libs.min.js.map -p relative",


I know you tried client/js/libs/*/*.js and that didn't work, but have you tried client/js/libs/**/*.js by any chance?

Alright, stupid question: do we want to uglify these? I don't think it matters too much in terms of performance.
EDIT: I also realize it makes things much simpler to load from the HTML file, so feel free to ignore if stupid comment :)

I don't think there's a particularly good reason to uglify them other than for ease of loading, but webpack can fill this role once #609 lands and then we can drop uglify.

astorije · 2016-09-23T05:30:29Z

package.json

@@ -17,9 +17,9 @@
  "scripts": {
    "coverage": "istanbul cover node_modules/mocha/bin/_mocha -r test/fixtures/env.js test/**/*.js",
    "start": "node index",
-    "build": "npm run build:font-awesome && npm run build:grunt && npm run build:handlebars",
+    "build": "npm-run-all build:*",


I am 99% sure we didn't use npm-run-all for a reason here. I think it was that we didn't want to use it too much, hoping to get rid of it sooner or later, but oh well. It's pretty cool that it supports *...

npm-run-all seems like a reasonable dependency at this point, but I can imagine simplifying things to a point where we can just use && (or & for parallel) and drop a dependency. It's simple enough for now though, I think.

astorije · 2016-09-23T05:43:01Z

First of, amazing, I am so happy someone tackled this!

Windows doesn't support globbing (client/js/libs/*.js doesn't expand to the list of js files in windows shell)

Guess what, I think we added AppVeyor right after the last mess up there as well :D

Ultimately I think these libs/ should be either moved to dependencies installed via npm, included in the main bundle by import, or removed altogether.

Uglify can then be part of the webpack build process, and this npm script will go away.

Agreed.

So I think it's OK to explicitly list out the subdirectories for now.

Added a comment to try with **.
EDIT: Their simple_glob function, which really should be called simple_glob_placebo, doesn't seem to support ** either, sigh...
So yeah, having the libs in the repo was always a non-permanent solution, and we've made progress on that, so I'm happy with the current solution unless we have a better idea.

nornagon · 2016-09-23T06:11:33Z

** doesn't even work on OSX, because npm scripts are executed with sh, not
bash, and sh doesn't support ** :(
On Thu, Sep 22, 2016 at 22:43 Jérémie Astori notifications@github.com
wrote:

First of, amazing, I am so happy someone tackled this!

Windows doesn't support globbing (client/js/libs/*.js doesn't expand
to the list of js files in windows shell)

Guess what, I think we added AppVeyor right after the last mess up there
as well :D

Ultimately I think these libs/ should be either moved to
dependencies installed via npm, included in the main bundle by import,
or removed altogether.

Uglify can then be part of the webpack build process, and this npm
script will go away.

Agreed.

So I think it's OK to explicitly list out the subdirectories for now.

Added a comment to try with *.
EDIT: Their simple_glob function, which really should be called
simple_glob_placebo, doesn't seem to support * either, sigh...
So yeah, having the libs in the repo was always a non-permanent solution,
and we've made progress on that, so I'm happy with the current solution
unless we have a better idea.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#628 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAKjAExccFWyDnnV7aR9YfYTSeVfUzHuks5qs2bmgaJpZM4KEExb
.

xPaw

Not cross-platform

xPaw · 2016-09-24T09:26:28Z

Perhaps include uglifyjs in your webpack PR? Then the glob issues will be gone, and simplify it.

nornagon · 2016-09-24T21:38:52Z

This is cross-platform (appveyor passed, as did the OSX + Linux travis builds). Folding it into the webpack PR is cleaner, though, I agree :)

nornagon · 2016-09-25T02:28:30Z

I looked into folding this into #609, but ran into some problems. The libraries we include via the uglified libs.min.js are not CommonJS modules; they simply inject themselves into the global namespace. Webpack works by following require() calls, so it won't work with the libs unless we convert them all to use require/import.

IMHO this is still a win for now because we get to drop Grunt. As we further refactor the client we can shift stuff in libs/ over to CommonJS style and phase out uglify.

astorije · 2016-09-25T04:28:46Z

Not cross-platform

What's your issue with this, @xPaw? It seems like AppVeyor is happy and @nornagon kept cross-platformism in mind.

xPaw · 2016-09-25T04:41:51Z

That's why:

ERROR: can't read file: client/js/libs/*.js

I get that by running npm build in powershell.

xPaw · 2016-09-25T05:13:08Z

Figured it out, had to run npm install...

👍

astorije · 2016-09-25T05:15:33Z

Awesome stuff, thanks @nornagon!

Move uglify invocation into npm scripts and remove grunt

xPaw approved these changes Sep 22, 2016

View reviewed changes

Move uglify invocation into npm scripts and remove grunt

62edd07

nornagon force-pushed the ungrunt branch from 8a328f3 to 62edd07 Compare September 23, 2016 03:53

astorije assigned astorije and xPaw Sep 23, 2016

astorije added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. second review needed Meta: Do Not Merge This PR should not be merged. labels Sep 23, 2016

astorije reviewed Sep 23, 2016

View reviewed changes

xPaw requested changes Sep 24, 2016

View reviewed changes

astorije removed Meta: Do Not Merge This PR should not be merged. second review needed labels Sep 25, 2016

astorije added this to the Next Release milestone Sep 25, 2016

xPaw approved these changes Sep 25, 2016

View reviewed changes

astorije merged commit 38175b1 into thelounge:master Sep 25, 2016

nornagon mentioned this pull request Sep 25, 2016

Use node-glob for globbing mishoo/UglifyJS#1305

Closed

astorije modified the milestones: Next Release, 2.0.1 Sep 28, 2016

astorije removed this from the Next Release milestone Sep 28, 2016

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#628 from nornagon/ungrunt

4a91e16

Move uglify invocation into npm scripts and remove grunt

xPaw unassigned astorije and xPaw Mar 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move uglify invocation into npm scripts and remove grunt #628

Move uglify invocation into npm scripts and remove grunt #628

nornagon commented Sep 22, 2016

xPaw left a comment

nornagon commented Sep 22, 2016

nornagon commented Sep 23, 2016

astorije Sep 23, 2016

astorije Sep 23, 2016

nornagon Sep 23, 2016

astorije Sep 23, 2016

nornagon Sep 23, 2016

astorije commented Sep 23, 2016

nornagon commented Sep 23, 2016

xPaw left a comment

xPaw commented Sep 24, 2016

nornagon commented Sep 24, 2016

nornagon commented Sep 25, 2016

astorije commented Sep 25, 2016

xPaw commented Sep 25, 2016

xPaw commented Sep 25, 2016

astorije commented Sep 25, 2016

Move uglify invocation into npm scripts and remove grunt #628

Move uglify invocation into npm scripts and remove grunt #628

Conversation

nornagon commented Sep 22, 2016

xPaw left a comment

Choose a reason for hiding this comment

nornagon commented Sep 22, 2016

nornagon commented Sep 23, 2016

astorije Sep 23, 2016

Choose a reason for hiding this comment

astorije Sep 23, 2016

Choose a reason for hiding this comment

nornagon Sep 23, 2016

Choose a reason for hiding this comment

astorije Sep 23, 2016

Choose a reason for hiding this comment

nornagon Sep 23, 2016

Choose a reason for hiding this comment

astorije commented Sep 23, 2016

nornagon commented Sep 23, 2016

xPaw left a comment

Choose a reason for hiding this comment

xPaw commented Sep 24, 2016

nornagon commented Sep 24, 2016

nornagon commented Sep 25, 2016

astorije commented Sep 25, 2016

xPaw commented Sep 25, 2016

xPaw commented Sep 25, 2016

astorije commented Sep 25, 2016