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

Support noDeploy option when use layers #759

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

naoyasugita
Copy link

This change allows use of noDeploy option with the layers.

Should resolve:
#577

@emann
Copy link

emann commented Mar 29, 2023

Thank you @naoyasugita! I need this as well, looking forward to it being merged.

@naoyasugita
Copy link
Author

Hi @pgrzesik - I'd be happy to check in with you when you're ready.

@kaiosyng
Copy link

I hope this gets merged soon!

Copy link
Collaborator

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Thanks for proposing the fix and sorry for not responding sooner. Please see my comment and let me know what do you think. Also, would be good if we could have a test case covering this.

lib/zipTree.js Outdated
* @param {Object} noDeploy
* @return {Promise} a promise offering the original JSZip object.
*/
function addTreeNoDeploy(zip, src, noDeploy) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need a new function just for that as most of the logic is copied over from the original addTree. I think it would be better to just have the optional argument of noDeploy to the original function. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review :)
You are right. I'll fix it along with the test.

Copy link
Author

Choose a reason for hiding this comment

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

@pgrzesik I'm done adding test and fixing the code. Please check it out.

@pgrzesik
Copy link
Collaborator

Hey @naoyasugita - it looks like the tests are currently failing. Could you please look into that whenever you have a chance? 🙇

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.

None yet

4 participants