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] Move preview-email to devDeps #453

Closed
felixmosh opened this issue Sep 20, 2023 · 8 comments
Closed

[fix] Move preview-email to devDeps #453

felixmosh opened this issue Sep 20, 2023 · 8 comments
Labels

Comments

@felixmosh
Copy link

Describe the bug

Since preview-email is for development purposes, there is no real good reason to make it a dependency of this lib.
It should be defined as devDep, and required within the condition that enables it.

@titanism
Copy link
Contributor

@felixmosh v12.0.0 of email-templates has been release to GitHub and npm with optional dependency of preview-email. In production environments it will console.error if you attempt to have previewEmail option with a truthy value, and in production environments it will throw.

Thank you for your feedback and patience @felixmosh

Consider supporting our efforts at https://forwardemail.net 🙏

https://github.com/forwardemail/email-templates/releases/tag/v12.0.0

@felixmosh
Copy link
Author

I've tested the new version, with Yarn, looks like it still installs it :|

Since this lib only passes users config.preview to the lib, I think that it's better to make config.preview the lib itself, WDYT?
Before
image
After
image

12 Mb of extra bloat

@titanism
Copy link
Contributor

You may need to use yarn install --ignore-optional or yarn workspaces focus --production. Also note that NODE_ENV=production should prevent installing devDependencies. We do not use yarn.

@felixmosh
Copy link
Author

Sure, my tests were with yarn workspaces focus --production

@titanism
Copy link
Contributor

We suggest to use pnpm or figure out the underlying reason and submit a PR. We don't suggest to pass the function, it's an anti-pattern.

You already have 661M, and you're upset about 12MB extra? I think there are other things to be concerned about with time.

@felixmosh
Copy link
Author

Yeah, that is right, but this is one of the big fishes :]

@titanism
Copy link
Contributor

Don't you need to regenerate (delete/recreate) your lockfile completely after a big change like this?

yarnpkg/berry#2425

@felixmosh
Copy link
Author

I've tried it, no luck

What is the antipattern by passing the a function / the lib instance?

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 a pull request may close this issue.

2 participants