search

maizzle / framework

Quickly build HTML emails with Tailwind CSS.
https://maizzle.com
MIT License
1.24k stars 49 forks source link

Overriding `process.env.NODE_ENV` has unintended side effects #717

Closed m5r closed 10 months ago

m5r commented 2 years ago

I'm using maizzle to programmatically render emails and I don't use any maizzle environment as my config is inlined in my code. So maizzle believes I'm using the local environment and it overrides process.env.NODE_ENV with an arbitrary value in https://github.com/maizzle/framework/blob/master/src/generators/output/to-string.js#L10 and in https://github.com/maizzle/framework/blob/master/src/generators/output/to-disk.js#L15
This is causing my code to behave unexpectedly because it relies on process.env.NODE_ENV to be production.

The quick fix I came up with was setting options.maizzle.env to process.env.NODE_ENV and creating two empty config files config.production.js & config.development.js but I would prefer for maizzle to not override my environment variables, especially not NODE_ENV.

Side note, I believe it would make more sense to include maizzle in the config files name (i.e. maizzle.config.production.js`), the current name is too generic.

cossssmin commented 2 years ago

Agree we should avoid this and not touch NODE_ENV but it will have to come in a major release as it could be a breaking change for other users 👍

Noticed something else when working through the issue though, currently the framework forces you to create a config.{env}.js file if you specify the env when using it programmatically:

https://github.com/maizzle/framework/blob/d021466bacfe3ec626dda90d0480860c2a5d288f/src/generators/config.js#L22-L28

This shouldn't be the case and it should simply fallback to the 'local' config.js or config.local.js that it computes before that. And even if that one is missing, worst case it would fallback to an empty object, as pretty much everything in Maizzle can (and should) work with some defaults.

This would also be a breaking change so I think we can group them together for Maizzle 5.

Not a big fan of the long config file names tbh, but maybe we could do maizzle.js and maizzle.production.js, not sure yet...

m5r commented 2 years ago

I can see why it could make sense to need a config file when using the CLI if the defaults are not enough but it shouldn't require a config file when used programmatically if the config is already passed as a parameter.

Yeah, it makes sense to group these two breaking changes together.