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

Extending custom path support #487

Merged

Conversation

dannegm
Copy link
Contributor

@dannegm dannegm commented Jul 6, 2022

DESCRIPTION

Implementing this plugin within a monorepo architecture where the .env file needed to be shared across different sub-packages, I ran into the situation that the plugin did not behave as expected when using a specific path using the safe mode, and the file defaults.

new Dotenv({
    path: '../../.env',
    safe: true,
    defaults: true,
}),

SOLUTION

Added fix to use path from settings in getDefaults() and getEnvs() functions. Also fixed a false positive test related to safe mode. All tests must pass successfully without decreasing the current coverage of the repository.

@dannegm
Copy link
Contributor Author

dannegm commented Jul 6, 2022

Issue related
#417

@mrsteele
Copy link
Owner

mrsteele commented Jul 6, 2022

This is great (and pretty clean as well).

A few questions:

  1. This is a BREAKING CHANGE yes? If you had your .env.defaults in the current directory and your .env file in another directory, this would break that.
  2. You changed a test, I think that shouldn't have changed, but also shows the issue with what I said above.

Thanks for this PR!

@dannegm
Copy link
Contributor Author

dannegm commented Jul 6, 2022

This is great (and pretty clean as well).

A few questions:

  1. This is a BREAKING CHANGE yes? If you had your .env.defaults in the current directory and your .env file in another directory, this would break that.
  2. You changed a test, I think that shouldn't have changed, but also shows the issue with what I said above.

Thanks for this PR!

Thank you for your review. Answering your questions:

  1. Yes, it could be, I suggest adding a flag where to choose if you want to use the same path for your.env.example and .env.defaults files
  2. Not, the issue with the test is that you are testing an empty file with another empty file, so, it is not showing the expected result that is comparing an empty file with a required var. That's why is a false positive

@mrsteele
Copy link
Owner

mrsteele commented Jul 7, 2022

@dannegm okay, i'll flag this as a "BREAKING CHANGE". I use an automatic release system, so would you mind adding a bit to the README about how the path works? We can probably add a note to the path definition, as well as explicitly mentioning the overriding opportunities on both the defaults and safe props?

@dannegm
Copy link
Contributor Author

dannegm commented Jul 8, 2022

@mrsteele I have already added the complementary documentation. Let me know if anything else needs to be done :)

@mrsteele
Copy link
Owner

mrsteele commented Jul 8, 2022

This looks great; and adding a section was a nice touch. 👍

One last note: do you think we should clarify defaults and safe props as well? I read it a few times and can’t decide if it’s clear the string argument overrides the full path to the file and doesn’t just append. Gimme your thoughts.

@dannegm
Copy link
Contributor Author

dannegm commented Jul 8, 2022

I think that the examples that I have provided are just enough to understand the final behavior. Within the pieces of code, it is specified what the result will be for both cases.

Also, while describing, it is specified that this affects both the path and the filename but yes, it would be nice to specify that only the .example and .defaults suffixes are appended to this.

Let me make a couple of corrections to see how it works better.

@dannegm
Copy link
Contributor Author

dannegm commented Jul 8, 2022

I just extended this note:

It is important to mention that this same path and filename will be used for the location of the .env.example and .env.defaults files if they are configured, this will only add the .example and .defaults suffixes respectively:

What are you thinking about it?

@mrsteele
Copy link
Owner

mrsteele commented Jul 8, 2022

@dannegm I just tried to fix provide a write-up and it was getting messy. Lets just leave those alone and go with what we got. Stand by.

@mrsteele mrsteele merged commit f5d79ee into mrsteele:master Jul 8, 2022
@github-actions
Copy link

github-actions bot commented Jul 8, 2022

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dannegm-adelaide
Copy link

Thank you so much for your support!

@mrsteele
Copy link
Owner

Thank you so much for your support!

Thank you @dannegm for the contributions! Good luck out there!

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 this pull request may close these issues.

None yet

3 participants