-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add gzip #49
base: master
Are you sure you want to change the base?
Add gzip #49
Conversation
@matyunya This is an awesome addition! I will make sure I get a good look at it tomorrow. Thanks again for the addition! |
index.js
Outdated
} | ||
|
||
function compressDir (dir) { | ||
var files = walkSync(__dirname + '/../../' + dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing your branch out, I am running into a few issues. I was trying to upload the Tests
directory within the project to s3 just to try it out and I ran into this
fs.js:946
binding.lstat(pathModule._makeLong(path));
^
Error: ENOENT: no such file or directory, lstat '/Users/nickbenoit/Documents/Projects/temp/s3-website/../../test/'
at Object.fs.lstatSync (fs.js:946:11)
at Object.lstatSync (/Users/nickbenoit/Documents/Projects/temp/s3-website/node_modules/graceful-fs/polyfills.js:297:22)
at walkSync (/Users/nickbenoit/Documents/Projects/temp/s3-website/index.js:523:13)
at compressDir (/Users/nickbenoit/Documents/Projects/temp/s3-website/index.js:555:15)
at putWebsiteContent (/Users/nickbenoit/Documents/Projects/temp/s3-website/index.js:566:5)
at /Users/nickbenoit/Documents/Projects/temp/s3-website/s3-website.js:168:7
at /Users/nickbenoit/Documents/Projects/temp/s3-website/index.js:498:9
at /Users/nickbenoit/Documents/Projects/temp/s3-website/node_modules/graceful-fs/graceful-fs.js:99:16
at /Users/nickbenoit/Documents/Projects/temp/s3-website/node_modules/graceful-fs/graceful-fs.js:43:10
at FSReqWrap.oncomplete (fs.js:135:15)
@matyunya Sorry to be slow, I have been swamped this week. I was testing your branch and ran into a few issues. I made a comment on the code that I think is at least partly responsible for the issue I was seeing. I am also happy to dig into the issue as I get more time throughout the weekend. Thanks again for the contribution. I am really excited about this feature. |
@nick-benoit14 Hey! So the problem was the context of execution: it's node_modules in my case, so I found no better solution than to check the base dir path. Still ugly though :) It should work now. |
@matyunya Thanks for pushing a fix. I still haven't had a chance to check it out. I will do my very best to get it tested tonight though. If all goes well I will get it merged and deployed! Thanks for your patience, I know it can be really frustrating to put up with busy slow maintainers. |
@matyunya Many apologies again for being slow. After thinking about it for a bit I kind of like the idea of copying the files then gzipping them instead of replacing them. I think it is easy to forget that it is going to gzip everything and inadvertently mess something up. What are your thoughts on that? Don't feel like you need to add that feature yourself, I am happy to work on it as well. I think it may just delay getting a new version out a bit. |
It's ugly but it works. Replaces original files with gzipped. Adds Encoding header.