-
Notifications
You must be signed in to change notification settings - Fork 2
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
Drop dotenv #70
Drop dotenv #70
Conversation
5d4533e
to
4b2c93b
Compare
4b2c93b
to
9539dfd
Compare
lib/eventboss/cli.rb
Outdated
@@ -36,7 +36,7 @@ def run | |||
private | |||
|
|||
def boot_system | |||
Dotenv.load | |||
Dotenv.load if defined?(Dotenv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see in dotenv change log Ruby >= 3.0 and Rails >= 6.1 are now required. Lock dotenv to ~> 2.0 if you are using an outdated Ruby or Rails version.
is the most breaking change. There might be a lot of apps with older Rails. I think that other than that it will be safe to keep the dotenv
dependency, but without specifying the version. Thanks to that we can avoid using if defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most breaking change is if an app do not had dotenv
as direct dependency, then dotenv
will be removed durning upgrade of eventboss
.
lock dotenv to ~> 2.0 if you are using an outdated Ruby or Rails version.
If an app uses Rails 6.1 then it won't be using the newest version of dotenv anyway, and official support for Rails 6.0
ended 9 months ago.
it will be safe to keep the dotenv dependency
Sure. I think it can be easier to push
I think i also need to conditionally |
Right now, we can't upgrade
dotenv
because of a gem conflict. So, instead of just loosening the dependency, I decided to cut it out completely. That said, ifdotenv
is around, our app will happily load it up.This aspec was previously brought up here
I don't know about what versions should it be/who should change it, so happy to hear how to do it