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

Events are triggered sequentially when the handlers execute async code #1768

Merged
merged 1 commit into from Mar 13, 2018

Conversation

Projects
2 participants
@kirrg001
Contributor

kirrg001 commented Feb 21, 2018

closes #1765

  • tests pass locally for all clients
  • added a test which proofs that the fix works
@ricardograca

This is mostly ok. There's just a small issue with the test that should be fixed.

m.off();
});
m.save(null, {method: 'insert'})

This comment has been minimized.

@ricardograca

ricardograca Feb 21, 2018

Member

Do you mind returning the call to save() here instead of calling ok() at the end? This ensures better consistency with the rest of the test suite and if the test fails the error will get caught correctly instead of being handled by the generic global onPossiblyUnhandledRejection() handler.

This comment has been minimized.

@kirrg001

kirrg001 Feb 21, 2018

Contributor

will update 👍

@@ -517,6 +517,33 @@ module.exports = function(bookshelf) {
});
});
it('ensure events are async', function(ok) {

This comment has been minimized.

@ricardograca

ricardograca Feb 21, 2018

Member

You call the events "async", but they seem more sync now since the next listener is only called after the first one resolves, so can you explain the async nature of this test?

This comment has been minimized.

@kirrg001

kirrg001 Feb 21, 2018

Contributor

will update 👍

@ricardograca ricardograca added this to To Do in Version 0.13.0 via automation Feb 21, 2018

@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Feb 21, 2018

This is mostly ok.

Thanks for review. Will update as soon as possible.

@ricardograca ricardograca moved this from To Do to In Progress in Version 0.13.0 Feb 21, 2018

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Feb 22, 2018

Use bookshelf npm fork
- i discovered two bookshelf bugs on 0.10.3
- bookshelf/bookshelf#1769
- bookshelf/bookshelf#1768
- we are currently locked to 0.10.3, because we saw a connection problem when updating to the latest knex+bookshelf
- using a tarball can trouble when installing deps
  - we saw this in the past
  - e.g. you have bookshelf already installed
  - now you switch to a tarball
  - neither yarn, nor npm are able to replace the dep
- require bookshelf-0.10.3
@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Mar 5, 2018

@ricardograca I've pushed a commit with your suggestions. Ready to squash + merge if you are ok with it 👍

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Mar 12, 2018

Use bookshelf npm fork
- i discovered two bookshelf bugs on 0.10.3
- bookshelf/bookshelf#1769
- bookshelf/bookshelf#1768
- we are currently locked to 0.10.3, because we saw a connection problem when updating to the latest knex+bookshelf
- using a tarball can trouble when installing deps
  - we saw this in the past
  - e.g. you have bookshelf already installed
  - now you switch to a tarball
  - neither yarn, nor npm are able to replace the dep
- require bookshelf-0.10.3
@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Mar 13, 2018

@ricardograca Hey. is there any chance that this get's released by the end of this week? 😁
Thanks :)

@ricardograca

This comment has been minimized.

Member

ricardograca commented Mar 13, 2018

I'd say probably not, but it won't be much later after that. There are still 3 issues left in the project for the next version, although I'm already looking into the table alias one.

You can go ahead and merge all of your approved PRs though.

@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Mar 13, 2018

You can go ahead and merge all of your approved PRs though.

👍

@kirrg001 kirrg001 changed the title from Fixed events being async to Ensure events are triggered sequentially when the handlers do async stuff Mar 13, 2018

Events are triggered sequentially when the handlers execute async code
closes #1765

- `triggerThen` was running in parallel before
  - event handlers were executed at the same time
  - that means you can't rely on an operation (or a change) which a previous handler executed
- use `mapSeries` instead
- added a test

@kirrg001 kirrg001 changed the title from Ensure events are triggered sequentially when the handlers do async stuff to Events are triggered sequentially when the handlers execute async code Mar 13, 2018

@kirrg001 kirrg001 merged commit b01f75d into bookshelf:master Mar 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Version 0.13.0 automation moved this from In Progress to Done Mar 13, 2018

@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Mar 13, 2018

I'd say probably not, but it won't be much later after that.

Till Monday would be very, very, very helpful. Ghost wants to ship a feature beginning of next week and i would love to avoid using a tarball link or a fork 😁 Thanks in advance!

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Mar 13, 2018

Use bookshelf npm fork
- i discovered two bookshelf bugs on 0.10.3
- bookshelf/bookshelf#1769
- bookshelf/bookshelf#1768
- we are currently locked to 0.10.3, because we saw a connection problem when updating to the latest knex+bookshelf
- using a tarball can trouble when installing deps
  - we saw this in the past
  - e.g. you have bookshelf already installed
  - now you switch to a tarball
  - neither yarn, nor npm are able to replace the dep
- require bookshelf-0.10.3
@ricardograca

This comment has been minimized.

Member

ricardograca commented Mar 13, 2018

Ok, if I'm unable to finished reviewing, merging and/or refactoring the last remaining PRs by Saturday I'll just move them to the next version and start preparing version 0.13.0 from what's already done which is already substantial.

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Mar 20, 2018

Use bookshelf npm fork
- i discovered two bookshelf bugs on 0.10.3
- bookshelf/bookshelf#1769
- bookshelf/bookshelf#1768
- we are currently locked to 0.10.3, because we saw a connection problem when updating to the latest knex+bookshelf
- using a tarball can trouble when installing deps
  - we saw this in the past
  - e.g. you have bookshelf already installed
  - now you switch to a tarball
  - neither yarn, nor npm are able to replace the dep
- require bookshelf-0.10.3

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Mar 21, 2018

Use bookshelf npm fork
- i discovered two bookshelf bugs on 0.10.3
- bookshelf/bookshelf#1769
- bookshelf/bookshelf#1768
- we are currently locked to 0.10.3, because we saw a connection problem when updating to the latest knex+bookshelf
- using a tarball can trouble when installing deps
  - we saw this in the past
  - e.g. you have bookshelf already installed
  - now you switch to a tarball
  - neither yarn, nor npm are able to replace the dep
- require bookshelf-0.10.3

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Mar 27, 2018

Use bookshelf npm fork
- i discovered two bookshelf bugs on 0.10.3
- bookshelf/bookshelf#1769
- bookshelf/bookshelf#1768
- we are currently locked to 0.10.3, because we saw a connection problem when updating to the latest knex+bookshelf
- using a tarball can trouble when installing deps
  - we saw this in the past
  - e.g. you have bookshelf already installed
  - now you switch to a tarball
  - neither yarn, nor npm are able to replace the dep
- require bookshelf-0.10.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment