search

dotmailer / dotmailer-magento2-extension

The official Dotdigital for Magento2 extension
https://dotdigital.com/integrations/magento
MIT License
48 stars 64 forks source link

Dont use node_modules when its not #580

Closed vandijkstef closed 2 years ago

vandijkstef commented 2 years ago

Had some issues with this module in the Magento Admin. Turns out you're delivering assets in a node_modules folder, while there isn't a package.json present. Like 'vendor' in php-land, 'node_modules' is assumed to be 'managed' with npm and a 'package.json' and often (at least in my case) globally added in the gitignore, thus excluded and -completely- breaking any JS interaction on the admin.

Please change the folder name to something else.

sta1r commented 2 years ago

Hi there, sorry this has caused you some issues. I believe we were advised to do this some time ago, in order to bypass some JS validation tests Magento-side (i.e. because colpick.js is third-party code). However a fix is overdue.

Do you have a workaround for this at the moment? I'd leave this until our next major release, but if it's a real pain we can push it in a fortnight.

vandijkstef commented 2 years ago

Thanks for getting back to this. Yes it's possible to work around the issue. Just got to either:

Problem came up since I build static assets on a seperate system, then check that into a 'deploy' repository which is what our non-development servers use. (Basically using git instead of any FTP-like sync tool, combined with near-0-downtime-deployment). So the assets where never included and fetchable from the pub or generated folder (can't remember which they actually got included in).

Since the JS assets returned HTTP 404, it was actually requireJS giving errors and halting execution, which led to any JS-based interaction not functioning at all. And sadly, theres too many of them in the Magento admin. Guess there is some blame in requireJS to this as well.

So safe to say it's due to a very specific workflow combination and I do have a workaround. It's fine to leave this as-is for now

sta1r commented 2 years ago

Hey, we've updated the code now, it's on our develop branch. Thanks for raising.