jbolda / gatsby-source-airtable

MIT License
216 stars 43 forks source link

Readme clarifications #1

Closed cbfrance closed 6 years ago

cbfrance commented 6 years ago

This is a great Gatsby plugin! I used it to replace gatsby-source-airtable when I realized it wasn't really designed for multiple tables.

I just had some quick feedback notes. I found a couple spots in the readme where some additional clarification would be good, the language is a little ambiguous.

If you are using a field type of Link to a Record, you may specify the field name in tableLinks (matching the name shown in Airtable, not the escaped version) and this plugin will create the graphql links for you.

• I think "you may specify" should be "you must specify". • Perhaps clarify it is the name of the linked-from field, not the name of the linked-to field, which must be specified in the tableLinks array.

Additionally you may provide a "mapping". This will alert the plugin that column names you specify are of a specific, non-string format. This is particularly useful if you would like to have gatsby pick up the fields for transforming, e.g. text/markdown.

• You "may provide X, which is helpful for Y" could perhaps be better phrased as "must provide X in order to Y." (Assuming it is actually required.) • Perhaps clarify explicitly that "string format is the default" (if that is true.)

Also:

• It's not clear to me when I would use the queryName or why I need the tableView. (This was also not clear to me on gatsby-source-airtable ... adding and removing it seems to have no effect in my case.)

Overall it would be super helpful to have a public example base and a project that referenced those field names. Perhaps if I have time I can work on this ticket, but I wanted to go ahead and file it since I just encountered these issues while getting started. Again, thanks for your work here! 👏

jbolda commented 6 years ago

Thanks for the comments, @chrisblow.

I think "you may specify" should be "you must specify".

I thought there might be an instance where you actually want the IDs from Airtable, but come to think of it, that is likely a very limited situation. Your wording later on may make sense to clarify that: "may provide X, which is helpful for Y".

Perhaps clarify it is the name of the linked-from field, not the name of the linked-to field, which must be specified in the tableLinks array.

Might you have accidentally flipped this? The linked-to field name should be used in tableLinks. Clarifying this makes sense to me.

You "may provide X, which is helpful for Y" could perhaps be better phrased as "must provide X in order to Y." (Assuming it is actually required.)

👍

Perhaps clarify explicitly that "string format is the default" (if that is true.)

Technically, it will be whatever graphql within gatsby "infers", but this is likely going to be a string or perhaps a number. Clarifying this would be helpful.

It's not clear to me when I would use the queryName or why I need the tableView. (This was also not clear to me on gatsby-source-airtable ... adding and removing it seems to have no effect in my case.)

Within Airtable, there exists your base which has table(s) and each table can have a view(s). A view can either be a different way to see your data, but it can also have embedded filters on your data. Specifying a view allows you to target a specific view (and possibly pre-filter if need be). I suspect, if you do not specify a view, that airtable just returns the default view.

Additionally, you have have a situation where you are including two separate bases each with a table that has the exact same name. With the data structure of this repo, both bases would fall into allAirtableLinked and you wouldn't be able to tell which from which table each record came. If you need to filter your records down to a specfic table in this situation, you can specify a queryName with which to accomplish this. It would only be for within your gatsby repo.

Overall it would be super helpful to have a public example base and a project that referenced those field names. Perhaps if I have time I can work on this ticket, but I wanted to go ahead and file it since I just encountered these issues while getting started.

I had actually started an example repo, public base and was writing a blog post. However, due to various circumstances, I decided to trash the blog post. I do still have the public base available and we could add a www folder that is automatically built on netlify with a simple example. I don't know if I really have time to get that going, but if you have some availability to help out I could likely make time to get the infrastructure in place for it.

Again, thanks for your work here!

Scratching my own itch, and just glad it helped someone else out! You're welcome!

cbfrance commented 6 years ago

Thanks this is helpful!

I now understand a bit more about GraphQL's inferred typing, when to use queryName, and the way Airtable views work with filtering (that's actually really helpful for me, because I do need to filter out some rows on the Airtable side, and I didn't realize I could save the filter as a view.) It does seem that the tableView can be blank if you are using the default view.

Definitely those would be good additions to the Readme.

Thanks for the example base!

One odd thing:

The linked-to field name should be used in tableLinks.

I am specifying the linked-from field and it is working. For example I have a "Posts" table which has an "Author" column that is linked to a "Person" table. I use "Author" in tableLinks for the Posts table. 🤔 ... Perhaps I am confused somehow. Here's my configuration if you are curious:

 tables: [
      {
        baseId: `appexamplekey`,
        tableName: `Person`,
        tableView: ``,
        tableLinks: [`Motivation`, `Group`]
      },
      {
        baseId: `appexamplekey`,
        tableName: `Posts`,
        tableView: ``,
        tableLinks: [`Author`]
      },
      {
        baseId: `appexamplekey`,
        tableName: `Motivation`,
        tableView: ``
      },
      {
        baseId: `appexamplekey`,
        tableName: `Group`,
        tableView: ``,
        tableLinks: [`Person`]
      },
      {
        baseId: `appexamplekey`,
        tableName: `Activity`,
        tableView: ``,
        tableLinks: [`Areas`]
      },
      {
        baseId: `appexamplekey`,
        tableName: `Area`,
        tableView: ``
      },
      {
        baseId: `appexamplekey`,
        tableName: `Motivation`,
        tableView: ``
      }
    ]
  }
cbfrance commented 6 years ago

Random note:

I found that omitting the tableView gives records in unpredictable order. So now I use

tableView: Grid view

when I need records in the same order as the table.

jbolda commented 6 years ago

I am specifying the linked-from field and it is working. For example I have a "Posts" table which has an "Author" column that is linked to a "Person" table. I use "Author" in tableLinks for the Posts table.

Actually, I was confusing myself! In my base, I happened to name the "linked-from" field the same as the linked-to table. When I first read your comment, I misunderstood it to mean the "linked-from" table.

I'd be happy to take a PR with any of these clarifying bits if you are willing!

cbfrance commented 6 years ago

I would like to make a contribution as time allows.

For now I want to add a couple notes:

• It seems that the tableLink name also must be different than the linked-to table name. I was getting an error TypeError: Cannot read property 'internal' of undefined until I realized this pattern. (I don't have an explanation, don't understand the underlying code — I might be misunderstanding the issue. But that is my observation based on experimentation.)

• Note to self (for future documentation) — when you create a tableLink to a table, make sure you are also bringing in that table. In my case I have an Activity with tableLink to Roles which is actually a Role table (they are named differently per above) and if I forget to separately include the Role table, I will get TypeError: Cannot read property 'internal' of null.

Hopefully we'll see new versions of Gatsby that have clearer errors in the future.

jbolda commented 6 years ago

For the user, I wonder if we catch these specific errors to explain them more. @chrisblow check out #5, I think your issues may actually be related. My documentation appears to be misleading if this is the fact.

jbolda commented 6 years ago

@chrisblow I just published a v2 beta (mostly to update for gatsby), and will need to add a few points in the docs for added functionality. Would you have time to add these clarifications or should I just plan to wrap them up prior to making v2 official?

cbfrance commented 6 years ago

Yep I’m still interested, perhaps I can do it tomorrow.

On Thu, Aug 23, 2018 at 5:45 AM Jacob Bolda notifications@github.com wrote:

@chrisblow https://github.com/chrisblow I just published a v2 beta (mostly to update for gatsby), and will need to add a few points in the docs for added functionality. Would you have time to add these clarifications or should I just plan to wrap them up prior to making v2 official?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jbolda/gatsby-source-airtable-linked/issues/1#issuecomment-415401104, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAcxvhgTd8m8O3IvOKHs7EraimRHsfTks5uTqPUgaJpZM4VBYL2 .

areohbe commented 6 years ago

@jbolda I'm taking on some feedback here as I'm not sure it's warrant it's own issue, and is likely due to some of the similar confusion @chrisblow had when getting started.

Let's say I have a table Fruit Stands in Airtable set up like:

Name Fruits
John Doe's Fruits apple orange ...
Jane Does' Fruits & More grapes banana

... and another table Fruits set up like:

Name Color
apple red
banana yellow
grapes purple
orange orange

In gatsby-config.js, I have:

tables: [
  {
    baseId: `fruitBase`,
    tableName: `Fruit Stands`,
    tableView: `Grid view`,
    tableLinks: [`Name`]
  },
  {
     baseId: `fruitBase`,
     tableName: `Fruits`,
     tableView: `Grid view`
   },
]

Based on the README and discussion above, I think that I have my config setup properly. However, if I try to query from the GraphQL browser interface for something like:

{
  allAirtableLinked(filter: {table: {eq: "Fruit Stands"}}) {
    edges {
      node {
        id
        data {
          Name
          Fruits {
            id
            data {
              Name
            }
          }
        }
      }
    }
  }
}

I receive the following error:

"message": "Field \"Fruits\" must not have a selection since type \"String\" has no subfields.",
jbolda commented 6 years ago

@areohbe Feel free to open up a new issue for this. I am assuming this is a contrived example, but Fruits should be plural in your query to match the column name. Otherwise it looks fine at first glance. If that isn't the issue, go ahead and open a new issue.

areohbe commented 6 years ago

@jbolda sorry for the typo, was was in a hurry earlier today - updated the example to be representative of what I was experiencing.

After a little bit of troubleshooting, I got tableLinks working but I used the field name of the field in the table I was linking to. I got a little mixed up between the readme and the discussion above. 😄 So in the example above, the correct config should actually be:

tables: [
  {
    baseId: `fruitBase`,
    tableName: `Fruit Stands`,
    tableView: `Grid view`,
    tableLinks: [`Fruits`]
  },
  {
     baseId: `fruitBase`,
     tableName: `Fruits`,
     tableView: `Grid view`
   },
]

I think?

While I am able to link a single filed, if I add multiple fields, something like:

tableLinks: [`Fruits`, `Hours`]

then GraphQL will throws errors on one of the fields indicating it is a string type. Maybe my query syntax is off?

jbolda commented 6 years ago

That is likely the first thing to look at. Please open a new issue that we may discuss there.

jbolda commented 6 years ago

@chrisblow I added some additional language to the readme (just in Github at the moment). Please open a new issue or PR if it needs further clarification. Thanks again for the input!