search

frappe / frappe_docker

Docker images for production and development setups of the Frappe framework and ERPNext
MIT License
1.42k stars 1.34k forks source link

node_modules in nginx images #498

Closed sunhoww closed 3 years ago

sunhoww commented 3 years ago

Just noticed node_modules take up 235M. Is there any reason it's included in the final nginx images? Is it somehow required by frappe down the line; maybe during runtime?

https://github.com/frappe/frappe_docker/blob/b670b4a9d52a4d5ce6115e7928de25251f1a612c/build/frappe-nginx/Dockerfile#L44-L46

revant commented 3 years ago

Yes! It is needed by frappe framework!

If you have native bench try,

cd ~/frappe-bench
ls -l sites/assets/frappe/node_modules

They are symlinked in this case.

Try removing it and ui will start failing.

Not just that! even the python gunicorn container needs the js and node_modules :facepalm:

sunhoww commented 3 years ago

Not just that! even the python gunicorn container needs the js and node_modules :facepalm:

Please pardon my naivete here. I figured node_modules are only required during the build stage of the nginx image build. So once the js are bundled, are node_modules still required?

So far I've seen the python container only needs the minified js during runtime. Possibly, is there any instance where frappe.client.get_js will send anything from node_modules?

Try removing it and ui will start failing.

I did just that. :smile: I'd been running a build since today. So far I haven't had any issues... yet.

revant commented 3 years ago

erpnext setup wizard works?

sunhoww commented 3 years ago

I didn't do a fresh install. I migrated a few existing sites. I'll check with a new site and get back.

revant commented 3 years ago

Text editor, code editor works?

sunhoww commented 3 years ago

Whoa. I did a text search for node_modules in the frappe application code and found a a bunch of frappe.require and links that do wants files from node_modules. Specifically

https://github.com/frappe/frappe/blob/version-13/frappe/website/doctype/web_form/templates/web_form.html https://github.com/frappe/frappe/blob/version-13/frappe/public/js/frappe/form/controls/code.js https://github.com/frappe/frappe/blob/version-13/frappe/public/js/frappe/form/print_utils.js https://github.com/frappe/frappe/blob/version-13/frappe/public/js/frappe/views/gantt/gantt_view.js https://github.com/frappe/frappe/blob/version-13/frappe/website/js/website.js

And no "Text Editor" fields no longer work.

So, I'll revert back to include node_modules.

ASIDE: The main gripe I had about node_modules was the long time it took to rsync the assets. I did change up the rsync options for it to make it work over an NFS mounted data volume. Maybe it was slow because of that. Then I saw node_modules, I thought - that's no place for build dependencies to live, that too exposed to the world. :smiley: So I removed the folder. I should have started with the text search.

revant commented 3 years ago

We're at least doing yarn --production=true

sunhoww commented 3 years ago

Maybe all required static assets in node_modules could be copied to another folder (the existing lib folder?) and referenced from there in application code. Or maybe from a cdn. But then it's up to the frappe team for such major changes.

For an instant, I thought about writing some patches and applying them during build. But, decided I shouldn't mess around with application code that much. Maintainability and what not. I can live with a slightly bigger image and slower container start up.

revant commented 3 years ago

We're living with 10+ year old software. Docker images didn't exist then. Probably vm images was the thing.

ERPNext worker image is ~1.5gb with all the py and os deps. It backs up and restores 1k tables for ERPNext.

In here, size doesn't matter!