jbolda / gatsby-source-airtable

MIT License
216 stars 43 forks source link

Row order not being respected in a view #95

Closed christopherjbaker closed 1 year ago

christopherjbaker commented 5 years ago

This is a duplicate of #54, but I cannot re-open it. I have however discovered the culprit

The data being returned by Airtable is in the order according to the view, but the order is not respected by the call to Bluebird.map, which is noted in the official docs. This is true even when concurrency is set to 1.

I don't see any official way to fix this with bluebird. Perhaps it could use mapSeries instead of map when concurrency is set to 1? Or not actually create any nodes and create them after (as the return value of map() is a promise which resolves to all the returned values).

Alternately, it could use a different library (such as async.mapLimit https://caolan.github.io/async/v3/docs.html#mapLimit) which does respect data order?

jbolda commented 5 years ago

Opening a new issue is perfect 👍 . Thanks for digging into this. I am not necessarily intent on keeping Bluebird (as annoying as it is to remove it, add it, and then remove it again).

I have two thoughts on this which might be worth having a discussion with core maintainers. Can we expect that nodes created in a certain order will be returned in that same order when queried? I think this was the case before we added the async considerations. If that is something we should not depend on (now or in the future), we may need to add a way to sort the rows. Second is regarding the limits of downloading remote things in general. I wonder if it may be worth exploring (if it is even possible) adding this functionality to the core gatsby-source-filesystem function createRemoteFileNode to limit to number of concurrent downloads.

@pieh, I don't suppose I can bug you for your input on the above paragraph?

christopherjbaker commented 4 years ago

I was able to resolve this myself with the following changes. Would you accept a PR of this?

Import both map and mapSeries from bluebird: const { map: mapParallel, mapSeries } = require('bluebird'); Use mapSeries when concurrency is 1, maintaing the current functionality for other cases: const map = concurrency === 1 ? mapSeries : mapParallel;

jbolda commented 4 years ago

I like that. Would love a PR with that :+1: . I think it's still worthwhile to add the row number on the node for sorting or special filtering, but that can be a separate PR.

zomars commented 4 years ago

Is this still active?

jbolda commented 4 years ago

@zomars are you wondering if this is still an issue?

zomars commented 4 years ago

Yes. I mean, the data is still fetched in the wrong order. I've tried to apply @christopherjbaker 's fix with no luck still.

jbolda commented 4 years ago

I suspect with all the async within this plugin and downstream in Gatsby core that it will be difficult to rely on any order. I do believe that we get a row id from our Airtable query though they could be used.

It seems it hasn't been enough of a pain point for someone to work on a PR though. Happy to accept PRs 😄

bernharduw commented 3 years ago

PR #282 seems to solve that issue in a very simple way, by adding the rowIndex to the results. It enables us to sort manually at least. Merging that PR would at least make a workaround possible.

github-actions[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had any recent activity for 30 days. It will be closed if no further activity occurs within 7 days. Remove stale label, comment, and/or apply "ongoing issue" label to prevent this from being closed. Thank you for your contributions.

mmassia commented 3 years ago

Why is this closed?

github-actions[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had any recent activity for 30 days. It will be closed if no further activity occurs within 7 days. Remove stale label, comment, and/or apply "ongoing issue" label to prevent this from being closed. Thank you for your contributions.

wahidshafique commented 2 years ago

PR #282 seems to solve that issue in a very simple way, by adding the rowIndex to the results. It enables us to sort manually at least. Merging that PR would at least make a workaround possible.

To add to this, you can also add an automation that reindexes on change. I did this

// change these names to pick a view:
let table = base.getTable('Levels');
let view = table.getView('Grid view');
let result = await view.selectRecordsAsync({ fields: [table.getField("Index")] });
let i = 0, len = result.records.length;
while (i < len) {
    const record = result.records[i]
    const cellIndex = record.getCellValue("Index")
    i += 1
    const expectedIndex = i
    if (record && cellIndex !== expectedIndex) {
        // we dont want this to run when the indices are matched, save a few millisecs
        await table.updateRecordAsync(record, {
            'Index': expectedIndex,
        });
    }
}
jbolda commented 1 year ago

Closing as it seems to have been address in #282.