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

Added field prefix table option #51

Closed
wants to merge 4 commits into from

Conversation

kevee
Copy link
Contributor

@kevee kevee commented Apr 23, 2019

When you have two tables in the same or different bases with the same field name, but different field types, then Graphql will not create the fields at all. We have a project where we pull from multiple bases we have little control over, and wanted to prefix the field names from different tables. This also helps prevent confusion when dealing with several different tables across projects.

This PR adds a new fieldPrefix option to tables that will get appended to any fields in a table. It works fine with linked table fields as well.

table

@jamessimone
Copy link
Collaborator

@kevee can you re-push your changes without whatever code formatting tool you have automatically formatting everything? Or change whatever you’re using (prettier?) to adhere to the existing formatting?

Previewing the changes there are a ton of extraneous changed lines and the PR should be clean with only the changes you’re making. Thanks!

@kevee
Copy link
Contributor Author

kevee commented Apr 23, 2019

@jamessimone Sorry didn't review the diff before committing. Fixed.

@jamessimone
Copy link
Collaborator

@kevee thank you kindly. I am traveling at the moment but hopefully one of us will be able to review this shortly. It’s a great workaround to this limitation in Airtable and always great to see contributions!

@jbolda
Copy link
Owner

jbolda commented Apr 24, 2019

Thanks for tackling this @kevee. Just to confirm my understanding, you are having the issues documented in #25 and #41, is this correct?

@kevee
Copy link
Contributor Author

kevee commented Apr 24, 2019

Yes, this should resolve both those issues.

@jbolda
Copy link
Owner

jbolda commented Apr 25, 2019

So my initial thought was to append a value to the node type. Is there more value in that or in your implementation (or both)?

For clarity, prepending (see Prefix) to the field name would let you query like the following:

allAirtable {
edges {
      node {
        data {
          PrefixField_1
          PrefixField_2
          ...
        }
      }
    }
  }
}

vs appending (see Suffix) on the type:

allAirtableSuffix {
edges {
      node {
        data {
          Field_1
          Field_2
          ...
        }
      }
    }
  }
}

Let me know if you need further clarification on anything.

@kevee
Copy link
Contributor Author

kevee commented Apr 25, 2019

I do like adding a suffix the node type instead of the field, since I am working with so many Airtable queries that look like:

allAirtable(
      filter: { queryName: { eq: "MyTableQueryName" } }
      ...
    )

But I think that it would make the Airtable plugin operate differently than, say, gatsby-source-filesystem, where since all the nodes are the same thing they get pulled under allFile and their docs specifically state that when you want to query by different sources, you use:

allFile(filter: { sourceInstanceName: { eq: "data" } }) {

However, unlike gatsby-source-filesystem, each airtable is basically creating a different schema, which I think would mean that they are different node types. If we add suffixes to the node type, it would also solve the minor, yet annoying, issue where in the GraphiQL interface it pulls all of the airtable fields (which in my case is over 100) for autocomplete. If the suffix was optional, there wouldn't be issues with migration to this new feature.

This PR was the result of my not wanting to pick sides on that issue, so instead it introduces what I'm thinking of as like SQL column name resolution notation (table.column), but I like suffixing the node types way better.

@jbolda
Copy link
Owner

jbolda commented Apr 26, 2019

Yea there is definitely some precedent to keep them all the same type, or at least there was early on. We would need to make this optional whichever way we choose so I'm not worried if this plugin is the only one to do it this way. Especially so as we essentially have a completely configurable schema.

So it sounds like we definitely want to do configurable type. Do you think it's still worthwhile to configure the field as well? It seems like you have more basis for an opinion on that than I. (@jamessimone or others feel free to express any opinions you may have as well.)

In other considerations, I think we do a minor bump for this. Also, if you wouldn't mind committing an small example for this so we can use it for "testing" in Travis that we don't accidentally break this going forward. We will also need docs, but I'm fine worry about that last.

@jamessimone
Copy link
Collaborator

@jbolda wish I could comment in greater depth but have been on an extended trip with a lot of traveling. I wanted to weigh in on the original version of what was presented just because the diff was much larger than what it should have been and that much I could see from even my phone, but I would want to take a more in-depth look at things prior to formally weighing in, which isn’t possible this week. I trust your judgement!

@kevee
Copy link
Contributor Author

kevee commented Apr 26, 2019

I think the field prefixes would not be needed if we could create different node types per table, since they were merely a way to work around the global namespacing of fields.

Since a separate node type would be a different feature entirely, I'll create a new branch and PR for that.

@jbolda
Copy link
Owner

jbolda commented Apr 30, 2019

@jbolda wish I could comment in greater depth but have been on an extended trip with a lot of traveling. I wanted to weigh in on the original version of what was presented just because the diff was much larger than what it should have been and that much I could see from even my phone, but I would want to take a more in-depth look at things prior to formally weighing in, which isn’t possible this week. I trust your judgement!

Oh no problem about that @jamessimone, didn't realize you were still traveling.

Thanks @kevee for jumping on that. Getting some 👀 on it.

@jbolda
Copy link
Owner

jbolda commented Jun 28, 2019

Closing this in favor of #52

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

3 participants