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

Promise rewrite #557

Merged
merged 29 commits into from Jun 9, 2018

Conversation

Projects
None yet
8 participants
@OmgImAlexis
Copy link
Member

OmgImAlexis commented Dec 6, 2017

This will be the PR used to track the Promise rewrite of Agenda.

100% of this will be breaking so don't hesitate to make big changes if it'll allow Agenda to work better when we release. We're going for 95% code coverage at launch and 100% by 2 weeks after.

@niftylettuce @simison @vkarpov15 @agenda/maintainers

@OmgImAlexis

This comment has been minimized.

Copy link
Member Author

OmgImAlexis commented Dec 6, 2017

In regards to milestones I don't know what we're going todo with this since 2.0.0 has #534 added to it. Maybe we need to goto 3.0.0 or release a 1.5.0?

@simison

This comment has been minimized.

Copy link
Member

simison commented Dec 6, 2017

In general it's better not to be afraid to bump major version when changes require it.

@OmgImAlexis OmgImAlexis modified the milestones: 2.0.0, 3.0.0 - Promise rewrite Dec 6, 2017

MongoClient.connect(url, options, (error, db) => {
if (error) {
// @TODO: This should be the mongodb instance
this.ctx.mongo = await promisify(MongoClient.connect)(url, options).catch(err => {

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Dec 6, 2017

Author Member

Anyone know why this would be returning undefined instead of the mongodb object?

.travis.yml Outdated
@@ -2,8 +2,6 @@ language: node_js
services: mongodb
sudo: required
node_js:
- "4"
- "5"

This comment has been minimized.

@simison

simison Dec 6, 2017

Member

Let's add this in a separate PR? It'll be more clear for folks reading commit history.

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Dec 6, 2017

Author Member

This whole branch will be replacing the old one so there isn't much point in that.

@OmgImAlexis

This comment has been minimized.

Copy link
Member Author

OmgImAlexis commented Dec 6, 2017

Looks like we may need to drop node 6 if we want async without babel.

@simison

This comment has been minimized.

Copy link
Member

simison commented Dec 6, 2017

Looks like we may need to drop node 6 if we want async without babel.

Node 6 is still very popular and LTS ends only April 2019.

Would it be more work to add Babel or perhaps something like caolan/async?

@OmgImAlexis

This comment has been minimized.

Copy link
Member Author

OmgImAlexis commented Dec 6, 2017

Looks like a lot of places will be jumping straight to v8 since it was released around the same time as v6 and they need to migrate from v4 anyways.

Should we vote?

@simison

This comment has been minimized.

Copy link
Member

simison commented Dec 6, 2017

Healthy community should rarely need to vote. ;-) I feel we can just weigh cons and pros?

Bottom line is; backwards compatibility should not be on the way of getting work merged. Adding Babel or a polyfill might slow us down too much.

That said, I'm sure with popularity of Agenda there are folks who need Node 6 support. They might be just okay using old version for now but we'd put them in difficult place if we next week find a security issue, although upgrading 6 → 8 was fairly painless. So it's just something to consider.

You have the steering wheel so I'll leave it up to you. :-) You could even publish this as a beta and see if anyone complains back. ;-)

@OmgImAlexis

This comment has been minimized.

Copy link
Member Author

OmgImAlexis commented Dec 6, 2017

If we do end up using babel I'd like to ship with the smallest amount of transforms possible.

@simison

This comment has been minimized.

Copy link
Member

simison commented Dec 6, 2017

If we do end up using babel I'd like to ship with the smallest amount of transforms possible.

Agreed. Maybe good if you work on this PR now as if it's Node 8 and then look back and re-consider once everything is working? You could add Node 6 into "allowed to fail" matrix without removing it from engines list.

.travis.yml Outdated
matrix:
allow_failures:
- node_js: 6
- node_js: 7

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Dec 6, 2017

Author Member

@simison any ideas here? I haven't used allow_failures before.

This comment has been minimized.

@simison

simison Dec 6, 2017

Member

Looks good otherwise but versions above are strings "6" and in matrix they're integers, might be it fails because of that.

See e.g. https://github.com/meanjs/mean/blob/482c38c287ff31688f0c9efa4ad4331009beeb40/.travis.yml#L4-L11

This comment has been minimized.

@simison
@simison

This comment has been minimized.

Copy link
Member

simison commented Dec 6, 2017

Bah, added crucial word to this. Somehow dropped it before:

backwards compatibility should not be on the way of getting work merged. Adding Babel or a polyfill might slow us down too much.

:-D Sorry if that confused.

OmgImAlexis added some commits Dec 10, 2017

@OmgImAlexis OmgImAlexis force-pushed the promise-rewrite branch from 418a7ef to 0867ac6 Dec 10, 2017

@agenda agenda deleted a comment from codecov-io Dec 10, 2017

@vkarpov15
Copy link
Collaborator

vkarpov15 left a comment

Good start, I just have a couple thoughts 👍

this._collection.deleteMany(query, (error, result) => {
if (cb) {
module.exports = function(query) {
return new Promise((resolve, reject) => {

This comment has been minimized.

@vkarpov15

vkarpov15 Dec 12, 2017

Collaborator

Why create a new promise when deleteMany() already returns one?

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Dec 12, 2017

Author Member

Result.n isn't a promise though and that's what we're resolving.

This comment has been minimized.

@vkarpov15

vkarpov15 Dec 13, 2017

Collaborator

so you can do return this._collection.deleteMany(query).then(res => res.result.n), no?

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Dec 13, 2017

Author Member

How's this?

module.exports = function(query) {
  debug('attempting to cancel all Agenda jobs', query);
  return this._collection.deleteMany(query).then(({result}) => {
    debug('%s jobs cancelled', result.n);
    return result.n;
  }).catch(err => {
    debug('error trying to delete jobs from MongoDB');
    return err;
  });
};

This comment has been minimized.

@vkarpov15

vkarpov15 Dec 21, 2017

Collaborator

Much better, just one detail: you need to throw err; instead of return err; in the catch() handler. The catch() handler must either throw an error or return a promise that rejects, otherwise the resulting promise will be resolved. Example:

const p = new Promise((resolve, reject) => reject(new Error('woops')));

// The below message will print even though `catch()` handler returned an error
p.
  catch(error => error).
  then(() => console.log('Not an error anymore!'));
job.save(cb);
return job;
const createJob = (when, name, data) => {
return new Promise(async resolve => {

This comment has been minimized.

@vkarpov15

vkarpov15 Dec 12, 2017

Collaborator

This async is unnecessary

README.md Outdated
@@ -55,13 +55,13 @@ agenda.define('delete old users', function(job, done) {
User.remove({lastLogIn: { $lt: twoDaysAgo }}, done);
});
agenda.on('ready', function() {
agenda.on('ready', async function() {

This comment has been minimized.

@vkarpov15

vkarpov15 Dec 12, 2017

Collaborator

Putting this async here feels weird. Perhaps instead of relying on agenda.on('ready') we should just bake that into agenda.start() so clients would just do

await agenda.start();

agenda.every('3 minutes', 'delete old users');
// ...

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Dec 12, 2017

Author Member

Oh good point. We don't really even need ready now that we return a promise from agenda.start(), right?

At least in regards to waiting for the queue to start, obviously people will still want to use the event for other tasks.

OmgImAlexis added some commits Dec 13, 2017

@simison

This comment has been minimized.

Copy link
Member

simison commented Jun 7, 2018

Are we planning on adding a transpile step?

We've been talking dropping Node.js 6 support.

@vkarpov15

This comment has been minimized.

Copy link
Collaborator

vkarpov15 commented Jun 8, 2018

TBH I'd just merge this and move on. Adding back support for Node.js v6.x would take 20 minutes, replace async/await with co and be done with it. This PR is a small but meaningful step in the right direction for this lib.

@karthikiyengar

This comment has been minimized.

Copy link

karthikiyengar commented Jun 8, 2018

So gentlemen/women, what is the consensus on this one?

If co is a priority, I'd be happy to add it - although I'm personally against introducing a library dependency made redundant by language features.

I'd really appreciate if we can hear a clear voice from the library owners/maintainers on what specifically is required to get this merged.

@wingsbob - Can you please clarify? I'm not sure if define needs to be a promise here unless define(...).then(() => agenda.now(..)) is a valid use case. If it is indeed going to be a promise, what should it resolve to?

@wingsbob

This comment has been minimized.

Copy link
Contributor

wingsbob commented Jun 8, 2018

If the job processing function you pass to define returns a promise, is what I meant to say (if that is any clearer)

simison added a commit that referenced this pull request Jun 8, 2018

Travis: drop Node.js versions 4, 5, 6 and 7 testing and testing for v…
…10 (#608)

* Drop Node.js versions 4, 5, 6 and 7 testing from Travis CI
* Add testing for Node.js version 10 LTS

This is pre-work for Promise rewrite: #557

https://github.com/nodejs/Release#release-schedule

simison added some commits Jun 8, 2018

Travis: drop Node.js versions 4, 5, 6 and 7 testing and testing for v…
…10 (#608)

* Drop Node.js versions 4, 5, 6 and 7 testing from Travis CI
* Add testing for Node.js version 10 LTS

This is pre-work for Promise rewrite: #557

https://github.com/nodejs/Release#release-schedule
@simison

This comment has been minimized.

Copy link
Member

simison commented Jun 8, 2018

We'll just make small updates to the README.md and try to get this merged over the weekend.

@vkarpov15

This comment has been minimized.

Copy link
Collaborator

vkarpov15 commented Jun 8, 2018

image

@simison

simison approved these changes Jun 9, 2018

Copy link
Member

simison left a comment

Long awaited. 👍

Will be in 2.0.0 release.

Kudos @OmgImAlexis @vkarpov15 @wingsbob and others who've been testing and working on this.

@simison simison merged commit b9ecca9 into master Jun 9, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk - package.json (OmgImAlexis) No new issues
Details

@simison simison deleted the promise-rewrite branch Jun 9, 2018

@karthikiyengar

This comment has been minimized.

Copy link

karthikiyengar commented Jun 9, 2018

Thank you so much guys 😁

@@ -320,13 +327,13 @@ By default it is `{ nextRunAt: 1, priority: -1 }`, which obeys a first in first

An instance of an agenda will emit the following events:

- `ready` - called when Agenda mongo connection is successfully opened
- `ready` - called when Agenda mongo connection is successfully opened and indeces created.
If you're passing agenda an existing connection, ou shouldn't need to listen for this, as `agenda.start()` will not resolve until indeces have been created.

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Jun 9, 2018

Author Member

ou should be you

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Jun 9, 2018

Author Member

indeces should be indices

- `ready` - called when Agenda mongo connection is successfully opened
- `ready` - called when Agenda mongo connection is successfully opened and indeces created.
If you're passing agenda an existing connection, ou shouldn't need to listen for this, as `agenda.start()` will not resolve until indeces have been created.
If you're using the `db` options, or call `database`, then you may still need to listen for `ready` before saving jobs. `agenda.start()` will still wait for the connection to be opened.

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Jun 9, 2018

Author Member

replace "listen for ready" with "listen for the ready event"

* @param {Object} data data to send to job
* @param {Function} cb called when schedule fails or passes
* @returns {*} job or jobs created
* @returns {Promise<Job>} job or jobs created

This comment has been minimized.

@OmgImAlexis

OmgImAlexis Jun 9, 2018

Author Member

Would this not be a "promise with a Job" or "an array of promises each resolving a job"?

OmgImAlexis added a commit that referenced this pull request Jun 10, 2018

Fix typos and mistakes in docs (#614)
@OmgImAlexis I think this is all the changes you requested in #557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.