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

Allow typeName and raw values to be configured for far greater robustness. #26

Closed
wants to merge 4 commits into from

Conversation

traversal
Copy link

@traversal traversal commented Dec 2, 2018

Addresses #25.

This PR includes three enhancements, which make this plugin much more robust for advanced use-cases:

Allow typeName to be specified for each table definition

typeName can now be configured for each table definition, which causes the GraphQL nodes to be defined via their own unique type names for each table configured in gatsby-config.js.

Adding the typeName avoids the problem where any two tables with the same field names but different data types will conflict with each other ( issue #25 ), which will create a lot of fairly unclear warnings when generating Gatsby nodes.

typeName is specified on a table-by-table basis, and if not present the standard ‘Airtable’ is used as before. For example, suppose we have two tables in AirTable called "Pages" to store a set of pages you want rendered in Gatsby, and each can have a "Person" linked to them, to display some kind of author perhaps. Then our config could look like:

{
  baseId: [AIRTABLE_BASE_ID],
  tableName: `Pages`,
  typeName: 'AirtablePage',
  tableView: `Grid view`,
  tableLinks: [
    `Person`,
  ],
  mapping: {
    Content: 'text/markdown',
  }
},
{
  baseId: [AIRTABLE_BASE_ID],
  tableName: `People`,
  typeName: 'AirtablePerson',
  tableView: `Grid view`,
  tableLinks: [
    `Page`,
  ],
},

This also has the superb side-effect of making GraphiQL much nicer, as the fields are no longer all mixed together between tables - you only see fields which are specific to each table, which is how I believe something like gatsby-source-contentful works already.

Raw value switches

The storage of raw values also causes a few cryptic warnings when the values of rows differ, so this commit also allows raw values to be switched off either globally:

{
  resolve: `gatsby-source-airtable`,
  options: {
    apiKey: `API_KEY`, // may instead specify via env, see below
    rawValues: false,
    tables: [
      ... etc 
    ]
  }
}

... or on a table-by-table basis:

{
  baseId: [AIRTABLE_BASE_ID],
  tableName: `Pages`,
  typeName: 'AirtablePage',
  tableView: `Grid view`,
  tableLinks: [
    `Person`,
  ],
  mapping: {
    Content: 'text/markdown',
  }
},
{
  baseId: [AIRTABLE_BASE_ID],
  tableName: `Person`,
  rawValues: false,
  typeName: 'AirtablePerson',
  tableView: `Grid view`,
  tableLinks: [
    `Page`,
  ],
}

By default, they will be stored as before.

Linked Fields issue

This PR also adds a try/catch around a piece of code used when processing linked fields - I found another very hard to debug issue occurs when you try to add a table link for a field which is not a linked record type. The try/catch block actually seems to work around this, but also logs a warning for the developer to fix the issue, as it could cause unexpected results. I couldn't quite parse how this code works, so I would be grateful for a bit of clarity around exactly why this try/catch fixes the issue.

Let me know if you'd prefer these issues split into three PRs - they're all semi-related, but understand if we'd prefer them split up. I'm using this version of the plugin in a fairly complicated Airtable setup and it works really well now. So well, I almost think some of these settings should maybe even be defaults - but we'll leave that up for discussion as it would cause back-compat issues.

Adding the typeName avoids a lot of problems where any two tables with the same field names but different data types will conflict with each other, and cause all sorts of unclear warnings when genertaing Gatsby nodes.

`typeName` is specified on a table-by-table basis, and if not present the standard ‘Airtable’ is used as before.

Raw values also cause a lot of warnings when the values of rows differ, so this commit also allows rawValues to be switched off globally, or on a table-by-table basis.
This prevents fatal (and very hard to debug) errors from occurring when you are trying to add a table link for a field which is **not** a linked record tyoe in Airtable.

The catch block logs a warning to the user so they have a good clue about where to track down the problem in Airtable itself.
@jbolda
Copy link
Owner

jbolda commented Dec 5, 2018

Hopefully, it won't be too onerous, but would you split it up? Only because I think there is value in having separate version bumps for each one of these. Especially so since you aren't confident in the try/catch block fix. If we do introduce a bug, a user can switch versions for the moment until it is fixed.

We will keep this PR open for the moment as we split them out. Feel free to do the typeName first as that is the biggest win and the most straightforward. Would you be able to expand a bit on the rawValues issue? I don't quite follow what the issue is.

Thanks again for your help here!


// ensure our type name is valid - remove non a-z chars and make the first letter uppercase
const cleanTypeName = (typeName) => {
return upperFirst(typeName.replace(/[^a-z]/ig, ''));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Gatsby takes care of the CamelCasing. Do we need to bother with upperFirst? One concern I have with automatically cleaning is that if we change what they input then it might be confusing when they go to query and it is different. Is it worth putting a warning that we changed it and recommend changing their value in gatsby-config to match?

Copy link
Author

@traversal traversal Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I'll review this to see if the upperFirst and cleaning can be removed. I believe though as I was developing these fixes that Gatsby was complaining about the formatting not being correct for the names, so I had to do the cleanup.

I like your idea about warning the user if we change it - that way we can ensure the build still succeeds, but encourage them to create a compliant typeName for each table too.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@traversal
Copy link
Author

No problems on the split-up I'll just need a bit of turnaround time to set it all up, as I'm quite busy this week!

I agree it's worthwhile though, in case we find any problems which need to be rolled back.

I'll also try to setup a test base in Airtable and another repo where I can demonstrate the rawValues issue. That might be resolved by the typeName thing as well, as I think it might have been complaining because the field type appeared to be different in each table. I believe it was something along the lines of one table had an "Image" field for attachments, but another table had an Image field with no data, and the no-data table was a assumed to be a string field type which conflicted with the image type, so Gatsby complained. It seemed to really tighten things up if we stopped storing rawValues.

I know it's been said before in other places but I find Gatsby's auto-inference of field types highly convenient, but also frustrating at times too.

I also ran into an issue earlier in the site I was developing, where it would have been really useful to store the original ID from Airtable's API in the node data as well. The use-case there was that I was trying to link the records together at the front-end, rather than use the table linking config. But I couldn't do it because I couldn't find those original row IDs, and you get an array of related row IDs in the result set if you don't use table linking. They may have been in the raw values all along, but in any case this might be a good separate PR to have in the future too, where we can just store the original API IDs - I can see that being useful for some folks.

I'm fairly confident in the try / catch block being a good fix, as I'm using it in a fairly complex base with about 12+ tables all linked together and haven't run into any issues at all. I'm mostly just not sure what the code inside that block does in the first place!

@traversal
Copy link
Author

Oh, and I'll try to include some suggested changes to the README file as well when I get all this setup, which we can refine together.

@jbolda
Copy link
Owner

jbolda commented Dec 6, 2018

No worries about busy, I get it :)

Hmm, well, I honestly can't remember what I was doing with the rawValues, and it might just be something I carried over from other plugins without any solid reason.

Ah yes, docs! I completely forgot to mention that. Agreed where you mentioned, RE: defaults, that we just stress the use case in the docs. Also to note, we have an examples folder in the readme that Travis runs daily just to make sure that nothing in Gatsby core changes/breaks without us knowing about it. Feel free to these use cases into that as well so we can keep them working. (I can copy your test base as well, just need to coordinate that a bit.)

@traversal traversal closed this Jan 6, 2019
@traversal traversal deleted the master branch January 6, 2019 11:54
@traversal
Copy link
Author

Hey @jbolda, Happy New Year!

I've just closed this for now in preparation for splitting into multiple PRs. Sorry this has taken me a while to get back to, was flat out in the lead up to the holidays and then also quite busy over the break.

I'll update this PR with links to the new PRs once I'm done :)

@jbolda
Copy link
Owner

jbolda commented Jan 7, 2019

Cheers @traversal, I know how it goes! Happy New Year to you as well.

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

Successfully merging this pull request may close these issues.

None yet

2 participants