search

forwardemail / email-templates

Create, preview (browser/iOS Simulator), and send custom email templates for Node.js. Made for @forwardemail, @ladjs, @cabinjs, @spamscanner, and @breejs.
https://forwardemail.net/docs/send-emails-with-node-js-javascript
MIT License
3.66k stars 339 forks source link

preview-email as dependency even though it is usually a project devDependency #394

Closed astik closed 3 years ago

astik commented 4 years ago

Since commit e6ca0cd, preview-email is defined as a dependency. From our project point of view, it is a transitive dependency through email-templates. From our project point of view, preview-email should be a devDependency.

The current situation leads to an oversized node_module in production.

npm install email-templates pug --save

rm -rf node_modules
npm install
du -sh node_modules
 50M    node_modules

rm -rf node_modules
npm install --production
du -sh node_modules
 50M    node_modules

Whether we use the production flag or not, email-preview is fetched inside node_modules.

npm install git://github.com/astik/email-templates.git#without-preview-email pug --save

rm -rf node_modules
npm install
du -sh node_modules
 30M    node_modules

Node modules is 20Mo lighter without email-preview.

npm install preview-email --save-dev

rm -rf node_modules
npm install
du -sh node_modules
 50M    node_modules

rm -rf node_modules
npm install --production
du -sh node_modules
 30M    node_modules

In dev (without production flag), we have the email-preview dependency and a 50Mo node_modules: this is expected. In production (with production flag), bundle is back to 30Mo as email-preview is not fetched anymore.

Having a lighter production node_module folder is better:

I cerated 2 repositories as example to reproduce the difference:

astik commented 4 years ago

What would be great:

niftylettuce commented 4 years ago

@astik are you sure that the 20mb is due to email-templates? did you have other packages installed in your example above?

astik commented 4 years ago

Yes, i'm pretty sure it is due to preview-email and its transitive dependency.

As you can check into https://github.com/astik/email-templates-test, there is a branch "as-peer-dependency-preview-in-dev" : https://github.com/astik/email-templates-test/commits/as-peer-dependency-preview-in-dev. As you can check in those 3 commits, there are only 2 dependencies : email-templates and pug ; nothing else: https://github.com/astik/email-templates-test/commit/ff707d5b56eec197370056c78a32667f7562880c In a separate commit, i add preview-email as devDependency: https://github.com/astik/email-templates-test/commit/ee80e2395d44774a5349a9e50f77982eec2dd0da

You also can try it by executing commands above. My fork of email-templates simply remove preview-email as dependency.

niftylettuce commented 4 years ago

Probably just due to all the babel compilation stuff so we can support older versions of Node. I can just remove Babel and should be way more lightweight.

astik commented 4 years ago

Problem i see is that i ship in production some dependencies that are only needed for dev. node_module is a problem that can be lighten but the fact that i ship too mush is another.

Maybe i'm overthinking it =/

niftylettuce commented 4 years ago

Nah, you're right in that these are larger than they need to be. I'm saying the Babel stuff is on our side, e.g. https://github.com/forwardemail/preview-email/blob/master/package.json#L23

astik commented 4 years ago

In my test repo, i added some npm list results. For example, for preview-email: https://github.com/astik/email-templates-test/blob/master/npm-list/as-dev-dependency.txt#L192. A lot of dependencies are not deduped, which means they are not needed by email-templates (mailparser@2.7.7, pug@2.0.4, open@6.4.0, dayjs@1.8.28 and the babel you already mentionned: babel/runtime).

niftylettuce commented 3 years ago

Out of the box people shouldn't have to do an extra step to get preview-email running locally

I think instead it might be best to try to strip out what's not needed in preview-email

If you want to PR this repo and add instructions for preview-email to README as well as modify source please do so, I'd prob accept that

astik commented 3 years ago

Yes, i can see your point. Still, the other around is also valid : "Out of the box people shouldn't have to do an extra step to remove unneeded package from production build". As of today, I have no solution ... i'm not sure that NPM can allow this use case in one step in a satisfying way =/