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

eachPage Promisified behaviour #69

Closed
yoadsn opened this issue Aug 19, 2018 · 12 comments
Closed

eachPage Promisified behaviour #69

yoadsn opened this issue Aug 19, 2018 · 12 comments

Comments

@yoadsn
Copy link

yoadsn commented Aug 19, 2018

@kasrak Thanks for this work.
Could you also provide a snippet showing how the eachPage of a Query object behaves when promisifed?

Since the promise versions are used when the last callback is omitted, for the eachPage version that would be the done method. (Although the callbackArgIndex is specified, it still points at the last argument which is the done callback).

Is this the correct promisified usage of the eachPage function?

const data = [];
await table.select({}).eachPage((records, next) => {
  data.push(...records);
  next();
});

If so, it is still a bit weird to have to use a callback and implement the iterator/accumulator. where the promise happinness cannot be used to handle the async io taking place.

perhaps a better API would return a cursor with a promisified "next" method on it.. (waving hands here)

const cursor = await table.select({}).eachPage();
while (cursor.hasMore) {
  const moreRecords = await cursor.next();
  ...
}

Thanks Again!

@kasrak
Copy link
Contributor

kasrak commented Aug 20, 2018

Yep, your usage above is correct! It's admittedly a bit strange, but we wanted the promisification to be non-breaking.

In the common case where you just want to fetch all pages, you can use the table.select({}).all() method which returns a promise that resolves once all pages are fetched: https://github.com/Airtable/airtable.js/blob/master/lib/query.js#L98

@kasrak kasrak closed this as completed Aug 20, 2018
@stevage
Copy link

stevage commented Sep 5, 2018

This looks broken to me on version 0.5.6

base('MyTable').select().eachPage()
    .then(records => {
        console.log('Doesn\'t get called.');
    })
    .catch(records => {
        console.log(records.length + ' records passed to the exception handler.');
    });

Result:

50 records passed to the exception handler.

but we wanted the promisification to be non-breaking.

Hmm. The library isn't even at 1.0.0 yet, and callbacks are ancient history. Having non-standard promise behaviour is pretty bad.

@EvanHahn
Copy link
Contributor

EvanHahn commented Sep 6, 2018

Is catch receiving the records despite there being no error? That does seem like a bug.

@stevage
Copy link

stevage commented Sep 7, 2018 via email

@kasrak
Copy link
Contributor

kasrak commented Sep 14, 2018

@stevage Yep that's a bug, but the correct behavior in your code example would be for it to throw an error saying that eachPage expects a callback (even when using promises). The promise returned by eachPage gets resolved when all pages are handled but it doesn't receive an argument. So if you want to collect all records:

const allRecords = [];
base('MyTable').select().eachPage((recordsInPage, fetchNextPage) => {
    // This callback gets called for each page of records that gets fetched.
    allRecords.push(...recordsInPage);
    fetchNextPage();
}).then(() => {
    // The promise is resolved when there are no more pages to fetch.
    console.log(allRecords.length);
}).catch(err => {
    console.error(err);
});

Or equivalently:

base('MyTable').select().all().then(records => {
    console.log(records.length);
}).catch(err => {
    console.error(err);
});

@stevage
Copy link

stevage commented Sep 14, 2018

Yeah, it turns out .all() is what I was really looking for, and that works fine.

@Vadorequest
Copy link

Agree with non-standard promise behavior is a bad thing, doesn't improve developer experience.

@tslater
Copy link

tslater commented Mar 5, 2020

@stevage, what does .all() do? Will it paginate and get the results for you?

@stevage
Copy link

stevage commented Mar 6, 2020

Heh, I don't remember, this was a while ago!

@robbiet480
Copy link

robbiet480 commented Mar 30, 2020

.all() doesn't actually work if you pass in options to the select. It will only get the first page of results. Whoopsies, turns out all actually just won't work if maxRecords is set. Server never returns a offset after the first. Setting pageSize to 100 without maxRecords seems to work fine now.

@yoadsn
Copy link
Author

yoadsn commented Apr 1, 2020

Since I see folks still interested.. Here is an implementation wrapper I use to have a "Paged Cursor" kind of behavior which is promised based.
For me - this makes it easier to iterate over a large recordset and do some async work on each page.

Here is the wrapper:

function loadFromAirTablePagedCursor(base, table, options) {
  const cursor = {};

  const doneCallback = error => {
    if (error) {
      cursor.reject(error);
    } else {
      cursor.resolve();
    }
  };

  const eachPageCallback = (records, fetchNextPage) => {
    cursor.nextPage = () => {
      return new Promise((res, rej) => {
        cursor.reject = rej;
        cursor.resolve = res;
        fetchNextPage();
      });
    };
    cursor.resolve(records);
  };

  cursor.nextPage = () => {
    return new Promise((res, rej) => {
      cursor.reject = rej;
      cursor.resolve = res;
      base(table)
        .select(options)
        .eachPage(eachPageCallback, doneCallback);
    });
  };

  return cursor;
}

And used like this:

const base = new Airtable({
  /*your appid + creds*/
});
const table = "Some Table";
// This is the AT options provided to the JS Client
const options = {
  sort: "",
  pageSize: "",
  maxRecords: "",
  filterByFormula: "",
  /*other stuff*/
}; 

// No network on next line - just an ability to paginate using promises
// encapsulated in a "cursor"
const atPageCursor = loadFromAirTablePagedCursor(base, table, options);
try {
  // nextPage return a promise that resolves to an array of Record objects.
  let atResultsPage = await atPageCursor.nextPage();
  while (atResultsPage && atResultsPage.length) {
    // Process this page.
    const records = atResultsPage;
    records[0].get('Some Field'); // For example

    atResultsPage = await atPageCursor.nextPage();
  }
} catch (err) {
  // Errors thrown from the nextPage call would be caught here.
}

hth

@subos2008
Copy link

Can we get the docs updated? Just lost a couple of hours searching for this - as I'm sure many other people do.

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

No branches or pull requests

8 participants