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

Proposal: enable usage of envalid outside node-js by dropping the dependency on dotenv #30

Closed
barbogast opened this issue May 18, 2017 · 13 comments

Comments

@barbogast
Copy link
Contributor

Hi,

I like the idea of envalid a lot and would like to use it in a react-native app.

Unfortunately dotenv doesn't work within the app since it tries to import the fs module which isn't available in the app. I was able to solve this by using react-native-config to get the configuration object. Now I would like to use envalid to validate this configuration object. But since envalid imports dotenv that's not possible right now.

One solution would be to entirely remove the dotenv dependency from envalid. So instead of

const env = envalid.cleanEnv(process.env, {...})

users would have to call

const env = envalid.cleanEnv(require('dotenv').config(), {...})

The option dotEnvPath could then be removed.

This change would allow the retrieval of the environment by arbitary libraries (like react-native-config) or even different sources (json-files, databases) without the implicit loading of the environment by dotenv.

Thanks
Ben

@barbogast
Copy link
Contributor Author

barbogast commented May 18, 2017

Hmm... completely removing dotenv would be a breaking change which would potentially force a lot of users to adjust their code. Another way to solve this issue would be to inline the require calls for fs and dotenv into extendWithDotEnv(). Then the breaking change could be avoided.

@af
Copy link
Owner

af commented May 19, 2017

Cool idea, I would also like to see the library be environment-agnostic. Adding dotenv is a relatively recent change, but it does make things significantly more convenient for the primary use case (server-side env validation). So I definitely want to keep it in, but am very much open to an option to disable it for use cases like yours

@af
Copy link
Owner

af commented May 19, 2017

master now has your suggested change, could you give it a try and let me know if it works for you now?

@barbogast
Copy link
Contributor Author

Thanks a lot for working on this :-)

Unfortunately it doesn't work. I didn't actually try my suggestion (sorry), and it turns out that the packager of react-native somehow analyzes the code before it is packaged and will stumble on the inlined require calls even without executing them. There are quite some issues in the react-native repository about this behavior but I couldn't find a nice solution.

So the only solution I can think of right now is to completely remove the dependency. But I could understand if you don't want to do this.

@af
Copy link
Owner

af commented May 19, 2017

Ah that's right, I remember that gotcha from RN. Can you try latest master locally, but change require('dotenv') to require('dot' + 'env')? (and do something similar for 'fs') I seem to recall that RN doesn't support dynamic require statements, but I'm not sure if it would throw an error in this case

@barbogast
Copy link
Contributor Author

Hehe, nice idea. Yes, I'll test it. I'm not at home today, so it will be tomorrow.

@barbogast
Copy link
Contributor Author

Allright, I got it working, but it isn't nice.

The require-calls work with

    const fs = require('f'+'s')
    const dotenv = require('d'+'otenv')

But then the next error occurs when chalk is required by reporter.js. chalk requires supports-color which requires has-flag which tries to do process.argv.indexOf. And process.args is undefined in react-native.

So I see the following solutions:

  1. Use a custom reporter and
  • either conditionally require reporter from index.js
  • or conditionally require chalk from reporter.js
  1. Try to get has-flag to handle process.argv being undefined.

Fixing has-flag is probably a good thing since it's a trivial change and it feels right that has-flag should be able to handle this case. Depending how the packages depend on each other (~ or ^) it could take a while until the change is available in chalk.

Which option do you prefer?

@SimenB
Copy link
Collaborator

SimenB commented May 22, 2017

Conditionally requiring the reporter can probably be done. Since you can provide your own, this might be a good thing either way.

I find it weird that react-native is so strict on process usage though. React has a widespread use of process.env, why can't it just set process.argv in the same way?

@barbogast
Copy link
Contributor Author

I just created a PR for has-flag. Depending if it is accepted we can add the conditional require for reporter.js.

@af
Copy link
Owner

af commented May 23, 2017

Looks like the has-flag PR isn't going forward. I can see the maintainer's point– this is kind of a flaw with how react-native only partially stubs out the process object.

Since this looks like it'd be a pretty invasive change, I'd recommend forking envalid instead and removing the parts that are problematic with RN. If RN fixes this issue down the road, we can revisit making envalid work better with it.

@af af closed this as completed May 23, 2017
@SimenB
Copy link
Collaborator

SimenB commented May 23, 2017

I think envalid can conditionally require chalk in order to make a workaround available

@barbogast
Copy link
Contributor Author

Alright, I just created a fork with the changes required to get it working in react-native. The changes are minimal but the solution doesn't look nice.

I just made a PR in case you want to pull it.

Anyway, thanks for your support!

@barbogast
Copy link
Contributor Author

I just created a new PR: barbogast#2

tuannm151 pushed a commit to BSSCommerce/shopify-envalid that referenced this issue Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants