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

Handle rejections in migration actions #148

Merged
merged 1 commit into from
Jan 11, 2018
Merged

Handle rejections in migration actions #148

merged 1 commit into from
Jan 11, 2018

Conversation

alem0lars
Copy link
Contributor

If an asynchronous up/down migration function is rejected, node-pg-migrate at the moment fails, causing a unhandled rejection error!

This patch adds support for action rejections.

Before they were resulting in unhandled rejection errors!
if (action.length === 2) {
action(pgm, resolve);
} else {
const result = action(pgm);
// result conforms to Promises/A+ spec
if (typeof result === 'object' && typeof result.then === 'function') {
result.then(resolve);
result.then(resolve).catch(err => reject(err));
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for contributing!
I would prefer .catch(reject)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not the same thing!

In fact if you .catch(reject) the error isn't passed to the reject call and thus you won't get the error message for the rejection.

Instead if you do .catch(err => reject(err)) you pass the error and when the rejection is handled it will print out the error message to the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is almost the same thing as .catch(reject) is equivalent to .catch((...args) => reject(...args))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try the difference in your code

Copy link
Contributor

Choose a reason for hiding this comment

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

Tried and got same result

Copy link
Contributor

Choose a reason for hiding this comment

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

Promise.reject(new Error('fail')).catch(console.log.bind(console));
Promise.reject(new Error('fail')).catch(err => console.log(err));

have same result...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@alem0lars
Copy link
Contributor Author

Is it possible to merge changes?

@dolezel dolezel merged commit e7db5dc into salsita:master Jan 11, 2018
@alem0lars
Copy link
Contributor Author

Can you release a new version with the changes?

Thanks a lot

@dolezel
Copy link
Contributor

dolezel commented Jan 11, 2018

It is already released as 2.15.0

@alem0lars
Copy link
Contributor Author

alem0lars commented Jan 11, 2018 via email

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.

2 participants