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

New: Patch for #570 #661

Closed
wants to merge 2 commits into from
Closed

Conversation

corollari
Copy link

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 New: Use woker-farm to handle webpack multi compile #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
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

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

NeoReyad and others added 2 commits May 19, 2020 11:11
This improves memory footprint of compilations using package
individually. By only extracting the properties we need from webpack
stats/result we can clean up a lot more memory every compile. This also
uses seperate processes to compile each function which can clean up all
the memory related to compilations and build multiple functions at same
time.
@racedale
Copy link

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)

@corollari
Copy link
Author

@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.

@pmcavoy89
Copy link

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.

@corollari
Copy link
Author

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

@j0k3r
Copy link
Member

j0k3r commented Feb 2, 2021

Any chance to rebase against master and fix conflicts?

@corollari
Copy link
Author

@j0k3r sure, I can work on that

@pmcavoy89
Copy link

@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.

@corollari
Copy link
Author

@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

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.

JavaScript heap out of memory when packaging many functions
5 participants