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

Unhandled rejection with fields that have the same name but different types #25

Closed
cbfrance opened this issue Nov 21, 2018 · 9 comments
Closed

Comments

@cbfrance
Copy link

cbfrance commented Nov 21, 2018

It's possible this is expected behavior somehow, or I'm configuring it wrong. Feedback welcome.

Problem

I have a base where different tables have the same field name, but different types. Sometimes (when one is an array/multiple choice and the other field is string/single choice) this is handled gracefully with a warning. But other times (when one is a number and one is a string) the error was an unhandled rejection that was difficult to track down.

Temporary solution

I had to change the Airtable field types to match, and in the future I'll have to manually check to make sure that new field types don't conflict with any of the existing fields.

Better solutions

• I was surprised that the field names conflict at all. Perhaps this could be avoided somehow.
• Alternatively, I would expect there to always be a more friendly warning about conflicting types, not an unhandled rejection.

Extra details:

The friendly warning was like this:

This is what happens with Array[string] type (multiple choice) vs. String (single choice) type conflicts.

(Ideally this warning would include the name of both tables involved — currently it only shows one of the table names.)

warning There are conflicting field types in your data. GraphQL schema will omit those fields.
Airtable.data.Sector:
 - type: array<string>
   value: [ 'Life Sciences' ]
 - type: string
   value: 'Materials'

The unhandled rejection:

This is what happens with a Number type vs. String type conflict.

error gatsby-node.js returned an error


  TypeError: Cannot read property 'publicProjectsAndOrganizations' of undefined

  - gatsby-node.js:306 graphql.then.result
    /Users/chrisblow/git/thedataguild.com/gatsby-node.js:306:21

  - util.js:16 tryCatcher
    [lib]/[gatsby-cli]/[bluebird]/js/release/util.js:16:23

  - promise.js:512 Promise._settlePromiseFromHandler
    [lib]/[gatsby-cli]/[bluebird]/js/release/promise.js:512:31

  - promise.js:569 Promise._settlePromise
    [lib]/[gatsby-cli]/[bluebird]/js/release/promise.js:569:18

  - promise.js:606 Promise._settlePromiseCtx
    [lib]/[gatsby-cli]/[bluebird]/js/release/promise.js:606:10

  - async.js:138 Async._drainQueue
    [lib]/[gatsby-cli]/[bluebird]/js/release/async.js:138:12

  - async.js:143 Async._drainQueues
    [lib]/[gatsby-cli]/[bluebird]/js/release/async.js:143:10

  - async.js:17 Immediate.Async.drainQueues [as _onImmediate]
    [lib]/[gatsby-cli]/[bluebird]/js/release/async.js:17:14


error UNHANDLED REJECTION


  TypeError: Cannot read property 'filter' of undefined

  - api-runner-node.js:274 Promise.mapSeries.catch.then.results
    [thedataguild.com]/[gatsby]/dist/utils/api-runner-node.js:274:42

  - util.js:16 tryCatcher
    [thedataguild.com]/[bluebird]/js/release/util.js:16:23

  - promise.js:512 Promise._settlePromiseFromHandler
    [thedataguild.com]/[bluebird]/js/release/promise.js:512:31

  - promise.js:569 Promise._settlePromise
    [thedataguild.com]/[bluebird]/js/release/promise.js:569:18

  - promise.js:614 Promise._settlePromise0
    [thedataguild.com]/[bluebird]/js/release/promise.js:614:10

  - promise.js:693 Promise._settlePromises
    [thedataguild.com]/[bluebird]/js/release/promise.js:693:18

  - async.js:133 Async._drainQueue
    [thedataguild.com]/[bluebird]/js/release/async.js:133:16

  - async.js:143 Async._drainQueues
    [thedataguild.com]/[bluebird]/js/release/async.js:143:10

  - async.js:17 Immediate.Async.drainQueues [as _onImmediate]
    [thedataguild.com]/[bluebird]/js/release/async.js:17:14
@jbolda
Copy link
Owner

jbolda commented Nov 22, 2018

I would call this expected, but not a great experience. Those errors are thrown in Gatsby core because we only have one overall "group" for Airtable. So we are essentially combining multiple Airtable sheets into one "sheet" in Gatsby, and Gatsby can't have mixed data. Assuming it hasn't changed from last I looked, it will decide the data type for each field based on the first data it receives.

That being said, we need to decide if/how it's best to show this to the developer. We could add an option to make more than one "group", but that makes things more complex if you use the pretty APIs (e.g. onCreateNode). Or perhaps we just search for this in our code and through a more appropriate error or warning rather than relying on the Gatsby core errors top five enough direction. Have any thoughts on how we should approach this? Would love a PR as well once we decide the best path forward 😍

@waywardm
Copy link

waywardm commented Nov 22, 2018 via email

@traversal
Copy link

Hey guys, see my PR above - I'd come across this very issue myself in a fairly complicated Airtable build, and had intended to submit a PR a week or so ago! But here it is :)

I went through a few iterations of this, but my fork of this is working very well now and should address the issues @chrisblow is seeing. As mentioned in the PR, this also addresses a few other things - happy to split it into multiple PRs if you'd prefer @jbolda.

I also concur with @chrisblow around the comment "I was surprised that the field names conflict at all. Perhaps this could be avoided somehow.". The default setup is likely to trip a lot of people up going forward, and it feels like each table in Airtable should be split into its own node type. As mentioned in the PR, this also stops the fields getting mixed up in GraphiQL, so each table only has its own fields, which is a far better experience.

Right now my version can't do this "globally", as this would require some kind of automatic inference of the correct type name for each table. That's problematic as it's preferable to use the singular form of the table name for the GraphQL node type name, but most people would probably name their Airtable tables in the plural form (well, I certainly do). We could use some kind of code to attempt to singularise the table names already fed in, but that felt a bit "magic" to me, and I preferred the idea of specifying each type in each table config.

@chrisblow would be interested to hear if using this version solves your issues.

@traversal
Copy link

traversal commented Dec 2, 2018

Also, as @waywardm mentioned, the implementation in the pull request will also prevent you needing to even filter for the table name in GraphQL. Each type is just queried via allAirtablePage, or allAirtablePerson at the root level (which would be the example queries you'd use if you had a config as shown in the PR).

@jbolda
Copy link
Owner

jbolda commented Dec 5, 2018

Thanks for all the work @traversal !

I am hesitant to make this a default especially since it has always been this way. With v2, we are just better enabled to take it further so I suspect that is why we haven't bumped into these issues prior. Maybe this is something we discuss for v3 perhaps, but trying to generally keep the major version pegged with Gatsby core. Migration between a switch and as the default is pretty minimal if we do decide to go that route later.

This seems to be the easiest fix compared to trying to catch errors and have the user change their Airtable columns.

@traversal
Copy link

Hey @jbolda no problems at all, thank you too for the work on this plugin. It's really enabling me to use Airtable as a pretty solid CMS for Gatsby. If they'd add some kind of basic support for Markdown formatting, it'd be basically perfect, but I digress... :)

Agreed on the defaults, as I don't want to break current installs. It's probably something where we can stress in the documentation that users should add the typeName if they use Airtable for more complex cases where multiple tables share the same field names. It really does make it more robust, and also works really well with the work you've already done on table links, which makes multi-hop table references quite easy to setup.

I'll make further comments related to the PR over there, thanks for reviewing.

@jbolda jbolda added the ongoing issue Prevents the issue from being closed for being stale. label Sep 10, 2019
@socmov
Copy link

socmov commented Feb 3, 2020

Hi @jbolda any chance you can release the solution in #51? It looks like it will solve the issue that people are facing. Also, it aligns to how a number of other CMS graphql integrations work.

@jbolda
Copy link
Owner

jbolda commented Feb 3, 2020

@socmov I believe the functionality you are looking for will come in #121, but no one has had the time to carry it over the finish line. There is an alpha version published that has some of the functionality depending on what you need.

@jbolda jbolda removed the ongoing issue Prevents the issue from being closed for being stale. label Feb 17, 2020
@jbolda
Copy link
Owner

jbolda commented Feb 17, 2020

We finished out the PR, and have published it in a beta version of 2.1.0. Feel free to give it a shot, and let us know if you run into any issues. If nothing arises in the near future, we will publish it to the latest tag.

@jbolda jbolda closed this as completed Feb 17, 2020
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

5 participants