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

Added support for environments with dotenv #88

Merged
merged 1 commit into from
Nov 20, 2017
Merged

Added support for environments with dotenv #88

merged 1 commit into from
Nov 20, 2017

Conversation

wesleytodd
Copy link
Collaborator

Closes #85.

@alendorff Want to check this out?

@maksnester
Copy link

@wesleytodd sure, I will check it today, thank you for notifying!

@maksnester
Copy link

maksnester commented Nov 6, 2017

Works fine!

But now I faced with another issue related to this warn message:

warning : if your migration returns a promise, do not call the done callback

I don't think that it's related to this PR, but I haven't seen such problems before (just because I didn't try migrate on Windows, I was previously blocked because I couldn't use .env files, so it's the first time when I tried it).

So, migration like this not working at all (pool.query is mysql2 function, accepts callback, returns promise):
For that case migrations are waiting infinitely after warning message.
module.exports.up = next => pool.query('<SOME SQL>', next);

And that failed with not readable error, I see error : [object Object] in console.
module.exports.up = next => pool.query('<SOME SQL>');

But that case works fine

module.exports.up = next => { 
  // no return for promise here
  pool.query('<SOME SQL>', next); 
}

@@ -17,10 +18,21 @@ program
.option('-t, --template-file [filePath]', 'Set path to template file to use for new migrations', path.join(__dirname, '..', 'lib', 'template.js'))
.option('-e, --extention [extention]', 'Use the given extention to create the file', '.js')
.option('-g, --generator <name>', 'A template generator function', path.join(__dirname, '..', 'lib', 'template-generator'))
.option('--env <name>', 'Use dotenv to load an environment file')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[name]?

maybe specify the default ('.env') here instead of further down as well?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for me, I think it would be nice to load default .env file from project root, even without that option, but keep --env <path> to specify custom path if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree with that behaviour 👍 that was how I originally imagined as well, as well how db-migrate does it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats actually a mistake. I meant to make a helper for this so that it wasn't a copy paste job. See the other files where it is [name]. Thanks for catching it.

@wesleytodd
Copy link
Collaborator Author

wesleytodd commented Nov 6, 2017

@alendorff It looks like your pool.query is returning a promise. As a preventative measure against calling the callback twice, it warns when you return a promise then call the callback. Just don't return with your arrow function, in this example cause you were using the "implicit return" feature of the arrow function.

EDIT: also of note, this is a breaking change release. So breaks are expected.

@maksnester
Copy link

@wesleytodd yes, I know that, that's why I showed examples. I just confused because of two things:

  1. it worked previously, when I tested it on linux (make sense if current stable version doesn't support promises).
  2. error like error : [object Object]

@wesleytodd
Copy link
Collaborator Author

Ahh, ok. Sorry for the confusion.

  1. Current "stable" would be 0.x, which does not support Promises. This was added in 1.0.0-BETA-1 I think.
  2. I am not sure what would cause that error. The logging in this package is not robust, it was on my list to fix but I never got around to it. So the error logging requires err to be something with a good toString. My guess is they are not doing an Error instance, but instead doing some custom "POJO" which looks like an error.

@rodgarcialima
Copy link

@wesleytodd yes, I know that, that's why I showed examples. I just confused because of two things:

  1. it worked previously, when I tested it on linux (make sense if current stable version doesn't support promises).
  2. error like error : [object Object]

Het @alendorff did you figure out how to solve this "if your migration returns a promise, do not call the done callback" problem?

Thank you

@maksnester
Copy link

@rodgarcialima sorry man, I have no idea what is this all about already 😄

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

Successfully merging this pull request may close these issues.

4 participants