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

fix(bundle): remove rebuild because it causes slow performance on big… #528

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

BorjaMacedo
Copy link
Contributor

When the project is large, rebuild causes memory problems and prevents deployment in github actions and other environments.

@jamesmhaley
Copy link

+1 This is also blocking our pipeline. Would be amazing to get this into this amazing project!!

@floydspace
Copy link
Owner

Hi @BorjaMacedo thanks for the PR...
but we need to be careful here, I see in the changes history we first introduced the rebuild then removed it, then again added, and now removing again:

https://github.com/floydspace/serverless-esbuild/pull/487/files
https://github.com/floydspace/serverless-esbuild/pull/491/files

I feel like better we need to introduce a new config option skipRebuild (or disableIncremental like we had it before v1.48.0), so everyone is happy.

Is it what you can implement in this PR?

@BorjaMacedo
Copy link
Contributor Author

Hi @BorjaMacedo thanks for the PR... but we need to be careful here, I see in the changes history we first introduced the rebuild then removed it, then again added, and now removing again:

https://github.com/floydspace/serverless-esbuild/pull/487/files https://github.com/floydspace/serverless-esbuild/pull/491/files

I feel like better we need to introduce a new config option skipRebuild (or disableIncremental like we had it before v1.48.0), so everyone is happy.

Is it what you can implement in this PR?

I have implemented it and it seems to work perfectly. If you need me to do something else I can do it.

Copy link
Owner

@floydspace floydspace left a comment

Choose a reason for hiding this comment

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

hey @BorjaMacedo , looks good, could you please also add the new property in readme options table with a short description what it does, and I will merge it.

@BorjaMacedo
Copy link
Contributor Author

BorjaMacedo commented Mar 7, 2024

@floydspace Done my friend! ;)

Copy link
Owner

@floydspace floydspace left a comment

Choose a reason for hiding this comment

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

@BorjaMacedo awesome thank you, merging it

@floydspace floydspace merged commit b5a233b into floydspace:master Mar 7, 2024
3 checks passed
Copy link

github-actions bot commented Mar 7, 2024

🎉 This PR is included in version 1.52.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants