Skip to content

feat(LMP-6728): patch memory leak in serverless-esbuild fork #2

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

Merged
merged 1 commit into from
Apr 22, 2025

Conversation

Ilya-Meer
Copy link
Collaborator

  • feat(LMP-6728): initial commit to align fork with our process
  • fix(LMP-6728): patch bundle file to remove memory leak

Overview

This PR is an attempt to resolve our monorepo build issues stemming from a memory leak in serverless-esbuild. With this patch, suggested in the Github thread, memory consumption should go back to normal.

This PR also removes some extraneous files from the upstream and brings the repo more in line with our process of doing things.

Key Changes

Checklist

Copy link
Member

@gregholland gregholland left a comment

Choose a reason for hiding this comment

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

This PR also removes some extraneous files from the upstream and brings the repo more in line with our process of doing things.

What's the thought process behind this? I don't think this is necessary, here's my reasoning:

  • The goal, if this fix works is to submit a PR upstream to fix the problem, then go back to using the OG version, not our fork. We should use their commit style, and leave things as is.
  • It's going to be a challenge to ever pull in upstream changes now that we've diverged
  • The hope is this isn't something we're taking over and maintaining ourselves.

You actual fix itself seems fine, although I'm not familiar with the problem.

@Ilya-Meer
Copy link
Collaborator Author

Ilya-Meer commented Apr 22, 2025

This PR also removes some extraneous files from the upstream and brings the repo more in line with our process of doing things.

What's the thought process behind this? I don't think this is necessary, here's my reasoning:

  • The goal, if this fix works is to submit a PR upstream to fix the problem, then go back to using the OG version, not our fork. We should use their commit style, and leave things as is.
  • It's going to be a challenge to ever pull in upstream changes now that we've diverged
  • The hope is this isn't something we're taking over and maintaining ourselves.

You actual fix itself seems fine, although I'm not familiar with the problem.

@gregholland Okay, no problem, I can remove all the changes other than the patch. Getting this merged into the upstream is going to be challenging because I'm not familiar with the project either so I need to first test if this fix works, then if it does, make sure that it conforms with the upstream's code style, other functionality, get tests running locally, add test cases etc. To me that is less of a priority than solving our specific problem, which we can do without doing all of that work.

I don't want to maintain this fork, and I also don't want to spend cycles on intimately familiarizing myself with someone else's project especially if the plan is to move off of Serverless v3 in the first place

This fix was suggested in the following thread on the upstream repo:

floydspace#388
@Ilya-Meer Ilya-Meer force-pushed the LMP-6728-fork-serverless-esbuild branch from 23274f9 to b7788a4 Compare April 22, 2025 21:05
@Ilya-Meer Ilya-Meer merged commit 6f322c9 into master Apr 22, 2025
0 of 3 checks passed
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.

2 participants