serverless-webpack icon indicating copy to clipboard operation
serverless-webpack copied to clipboard

Published 20 hours ago • verified publisher icon serverless-heaven

New: Patch for #570

Open corollari opened this issue 4 years ago • 8 comments

What did you implement:

Recently I ran into the problem described in #299 and after testing several possible solutions (see this issue comment for a more detailed rundown of what I tried) I ended up using the fix implemented in #570. However, I ended up having some problems with the watch command used by serverless-offline. This PR implements a patch that fixes those issues and gets it working properly.

Closes #299

How did you implement it:

I made the script that is responsible for the watch command compatible with the rest of the changes made in the PR. Note that I didn't make the watch command use worker-farm for compilation when watching the changes, as it currently works fine without it.

How can we verify it:

  1. Verify that #570 works fine (this PR depends on it)
  2. Setup a new project that uses serverless-offline and try to spin a new local version with this PR's code, it should work fine

Todos:

  • [ ] Write tests
  • [ ] Write documentation
  • [ ] Fix linting errors
  • [ ] Make sure code coverage hasn't dropped
  • [ ] Provide verification config / commands / resources
  • [x] Enable "Allow edits from maintainers" for this PR
  • [x] Update the messages below

Is this ready for review?: Yes Is it a breaking change?: NO for #570, YES for the current package

corollari avatar Dec 01 '20 17:12 corollari

Just needed to use this by doing npm install -D serverless-heaven/serverless-webpack#pull/661/head to get our builds to work. We also use serverless-offline and #570 didn't work when we went to run tests.

Key thing (mentioned in the issue comment but not here) must remove use of webpack-node-externals. Which results in node_modules no longer being an included folder for individually packaged lambdas, and rather bundled directly in the js file. This will make it harder in the future to inspect a build artifact and confirm which version of a dependency is used by a lambda (for security scans, etc)

racedale avatar Jan 12 '21 19:01 racedale

@racedale Thanks for the feedback. I proposed a solution for the problem you mentioned with webpack-node-externals here, however I currently don't have the bandwith to implement it.

corollari avatar Jan 13 '21 03:01 corollari

Is there a way we can break down the work in the ToDos you have listed so we can get this hopefully reviewed and merged? My project needs this too.

pmcavoy89 avatar Feb 01 '21 20:02 pmcavoy89

@pmcavoy89 The todo list is just a standard list that came with the PR template, the PR could just be merged as-is.

corollari avatar Feb 01 '21 21:02 corollari

Any chance to rebase against master and fix conflicts?

j0k3r avatar Feb 02 '21 05:02 j0k3r

@j0k3r sure, I can work on that

corollari avatar Feb 02 '21 12:02 corollari

@corollari I get that. I was wondering if I could take any of the items in the ToDo list off your plate to help get this merged in, if we need to that list. I know you had mentioned not having a ton of bandwidth.

pmcavoy89 avatar Feb 03 '21 16:02 pmcavoy89

@pmcavoy89 Oh sorry, I didn't understand you. Yeah I'd really appreciate some help with:

  • Rebasing against master
  • Implementing the use of a fork syscall in order to solve the problems we are currently facing with non-dynamic config files
  • Adding a flag that toggles the features provided in this PR

corollari avatar Feb 03 '21 16:02 corollari