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

feat(zip): make bestzip an off-by-default option #187

Merged
merged 1 commit into from
Sep 18, 2021
Merged

feat(zip): make bestzip an off-by-default option #187

merged 1 commit into from
Sep 18, 2021

Conversation

troyready
Copy link
Contributor

bestzip preserves file timestamps, which causes function zips to be different after every esbuild.

Because there are scenarios where native js zipping is unacceptably slower than native system zip, the option to use it is preserved as an option.

Fixes: #186

bestzip preserves file timestamps, which causes function zips to be
different after every esbuild.

Because there are scenarios where native js zipping is unacceptably
slower than native system zip, the option to use it is preserved as an
option.

Fixes: #186
@troyready
Copy link
Contributor Author

FYI @vamche -- I'm proposing a change here that would require a small config change to preserve your use of system zip.

@vamche
Copy link
Contributor

vamche commented Sep 18, 2021

Hey @troyready, thanks for the PR. Looks good to me.

I have suggested a similar approach in the past but we didn't take it forward because of some concerns. Here is the related thread/comment #150 (comment). Could you please check with @olup?

@troyready
Copy link
Contributor Author

Thanks for the background, that is helpful.

FWIW, in regards to that exact comment: we wouldn't be able to use bestzip.nodeZip as is, because it suffers from the same timestamp issue. It might not be unreasonable to check in with them on supporting it.

To olup's comments, I'm fine either way. I do like the simplicity of just having a single implementation (without another config option), but it's going to be a matter of opinion on whether that's the right tradeoff.

Copy link
Owner

@floydspace floydspace left a comment

Choose a reason for hiding this comment

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

@troyready Looks good to me as well. thanks that documented it. as soon @olup approves as it concerns his comment, I merge it

@olup
Copy link
Contributor

olup commented Sep 18, 2021

Well, I am partial to a simple implementation. And for me, the main thing is to have reliable timestamps - a problem that I did not know about before. However because the native zip seems to be an important feature for some users, and as we started supporting it, I am fine with an optional parameter that lets one opt-in native zipping (reliable timestamps should stay the default).

So I am good with this PR.

@floydspace floydspace merged commit 10f7b99 into floydspace:master Sep 18, 2021
@github-actions
Copy link

🎉 This PR is included in version 1.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated zip files always have new timestamps and force stack updates
4 participants