fix excludeNodeDevDependencies method: dev modules not excluded on invalid package #7368
Conversation
…valid package missing package.json
darko1979
changed the title
There was a problem hiding this comment.
@darko1979 makes sense. Great thanks for that fix. I've just proposed one improvement, to avoid eventual side effects.
|
|
||
| return globs.concat(baseGlobs); | ||
| }) | ||
| .catch(() => globs); |
There was a problem hiding this comment.
To reflect properly if/else, let's pass this catch callback as second argument to previous then (otherwise it also handles errors which may happen in first then callback and that can impose nasty, difficult to debug issues)
There was a problem hiding this comment.
@medikoo Not really sure if I understood you correctly. If I'm not mistaken, the catch block I added only handles the error from readFileAsync it does not handle errors of any previous then blocks.
There was a problem hiding this comment.
it does not handle errors of any previous then blocks.
@darko1979 it handles. To ensure it'll only handle error from readFileAsync it needs to be attached directly to promise returned by readFileAsync (and not to result of then() in which we process it's result)
There was a problem hiding this comment.
@medikoo I understand now, I corrected it by your suggestion
What did you implement
This PR fixes the problem dev dependencies not excluded which produces large zip packages.
We had deployment issues with out project that prevented us from deploying a package to aws lambda because of the zip package size. After a few days of debugging I narrowed it to a problematic node_modules package that had it's own dependencies and one of them had an empty folder without package.json. This caused excludeNodeDevDependencies method to fail when trying to read package.json file because there was no catch on readFileAsync which caused this method to stop and return empty array so no modules were excluded.
Probably the cause of the issue #5396
Is this ready for review?: YES
Is it a breaking change?: NO