Skip to content

Conversation

@fhemberger
Copy link
Contributor

@jbergstroem
Copy link
Member

LGTM

@fhemberger
Copy link
Contributor Author

@nodejs/build So can we merge this?

@jbergstroem
Copy link
Member

Happy to merge, just wanted to give it some time seeing how I was the only one ack:ing.

@rvagg
Copy link
Member

rvagg commented May 10, 2016

it probably should be && not ;, perhaps it should just be one npm script so you control it all in node/nodejs.org? npm run deploy

@fhemberger
Copy link
Contributor Author

Updated to npm run deploy.

fhemberger added a commit to nodejs/nodejs.org that referenced this pull request May 11, 2016
@rvagg rvagg merged commit e9bc0ae into nodejs:master May 11, 2016
@rvagg
Copy link
Member

rvagg commented May 11, 2016

lgtm

@rvagg
Copy link
Member

rvagg commented May 11, 2016

> find -E build -type f -regex '.*(html|css|js|xml|json)$' -exec gzip -kf9 {} \;

find: unknown predicate `-E'

tested on osx and not linux? reverting on the server until you give the 👍 here that it's ready

@jbergstroem
Copy link
Member

Urgh. Sloppy reviewing on my end. I'll revisit too.

@jbergstroem
Copy link
Member

Shouldn't that really just be find build/ -type f ...?

@fhemberger
Copy link
Contributor Author

Sorry for that!

Replaced the command with find build -type f -name '*.html' -o -name '*.css' -o -name '*.js' -o -name '*.xml' -o -name '*.json' -exec gzip -kf9 {} \; – a bit longer, but no incompatible regex switches. Also tested this on Linux as well.

nodejs/nodejs.org@ff6c50f

@fhemberger fhemberger deleted the patch-1 branch May 11, 2016 15:50
@rvagg
Copy link
Member

rvagg commented May 12, 2016

@fhemberger OK, I've switched it over to deploy now but I don't think it's quite working right yet, my guess is that it's only -name '*.json' that's getting picke up cause I see site.json.gz but the others are plain.

@rvagg
Copy link
Member

rvagg commented May 12, 2016

Indeed ...

find . -type f -name '*.html' -o -name '*.css' -o -name '*.js' -o -name '*.xml' -o -name '*.json'

prints them all but

find . -type f -name '*.html' -o -name '*.css' -o -name '*.js' -o -name '*.xml' -o -name '*.json' -exec echo '{}' \;

prints only the .json files, so -exec must apply only to the last -name

this works tho:

find . -type f \( -name '*.html' -o -name '*.css' -o -name '*.js' -o -name '*.xml' -o -name '*.json' \) -exec echo '{}' \;

sort of like a grouped ||

@fhemberger
Copy link
Contributor Author

@rvagg Okay, tested find build -type f \( -name '*.html' -o -name '*.css' -o -name '*.js' -o -name '*.xml' -o -name '*.json' \) -exec gzip -kf9 {} \; again, seems to work fine.

nodejs/nodejs.org@3716408

@rvagg
Copy link
Member

rvagg commented May 12, 2016

neat, appears to be working: https://nodejs.org/en/index.html.gz

@jbergstroem can we confirm somehow that nginx is actually using these files? or do we just take it for granted?

@jbergstroem
Copy link
Member

@rvagg I've used it previously -- more info here: http://nginx.org/en/docs/http/ngx_http_gzip_static_module.html. I guess we could set up a test environment to prove it works?

@rvagg
Copy link
Member

rvagg commented May 15, 2016

Nah, let's not go overboard, it's just a shame we can't easily run a command and confirm that all this work actually achieved something.

@jbergstroem
Copy link
Member

We consume less cpu than before at least.

@rvagg
Copy link
Member

rvagg commented May 15, 2016

well that's certainly something, good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants