-
Notifications
You must be signed in to change notification settings - Fork 78
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
move sourcemap to rollup #2501
move sourcemap to rollup #2501
Conversation
Just a note this may affect functionality --- so side effects would need to be assessed https://rollupjs.org/configuration-options/#output-manualchunks |
scratch that manual chunking wasnt necessary locally |
I think you can close this. Your other PR worked great 🙂 |
I was going to maybe try figuring out how to test the sourcemap sometime this week. I think this is the preferable option if it works isn't it? |
Oh, I was thinking that the preferable option is to NOT mess with the source maps if we can help it! |
@Duncan-Brain can you resolve the conflict ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change seems to work. Let's resolve the conflict with integration
.
I was able to run a Docker build with 256m of memory associated.
docker build --memory 256m --progress=plain --no-cache --tag litefarm/webapp --file packages/webapp/prod.Dockerfile packages/
Attached the build log:
docker-build.log
If the --memory 256m was necessary on the docker statement maybe this PR is no longer working as intended. This was meant to be an alternative to having to specify memory allocation. It was a few months ago and the fix we implemented is working right now. I have this "Fix" here but do not fully understand how to differentiate/evaluate between the sourcemap created by vite and the one created by rollup. So... The merge commit is in (thus the consent text changes) but if this is not a better solution to specifying the memory we can also close it and say "this is not better". And instead if there are some targeted places we can implement the suggestion in your build log --- that also could be better :
|
@Duncan-Brain no, the |
I was able to run the build limiting the docker build process to 32m systemd-run --quiet --user --scope -p MemoryMax=32M -p MemorySwapMax=0 docker build --memory 32m --progress=plain --no-cache --tag litefarm/webapp --file packages/webapp/prod.Dockerfile packages/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏾
For the record, I was not able to reproduce the problem in the |
Closing issue - fix seems to work but the necessity of it is not clear. Defer to future fix of root problem on the following Jira ticket. Please see LF-3348 |
Description
This is another potential fix for the memory leak. Essentially it was found on the same vite issue: vitejs/vite#2433
Sourcemap: false also works but we want to keep it for debugging.
Sourcemap would need to be verified functional.
PR links: Fix#1 - #2495, Fix#2 - #2496, Fix#3 - #2497
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: