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

Option to make tables separate node types #52

Merged

Conversation

kevee
Copy link
Contributor

@kevee kevee commented Apr 26, 2019

Per conversation in #51, this PR:

  • Adds a new createSeparateNodeType boolean option to tables
  • When in use, it appends the table query name, or if unavailable, the table name to the Airtable node type (so a table Fruit would be under allAirtableFruit)

Copy link
Owner

@jbolda jbolda left a comment

Choose a reason for hiding this comment

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

A boolean, interesting. I think I do like it. It makes queryName actually useful still. We will want to drill this down into the buildNode function on L306+. Those AirtableField types hold the nodes built from mapping. If we don't drill it down there, we will still potentially get conflicting types when using mapping.

Looks like this should keep things backwards compat 👍 .

@kevee
Copy link
Contributor Author

kevee commented Apr 30, 2019

Good catch, added.

@jbolda
Copy link
Owner

jbolda commented May 1, 2019

Hmm, well Travis is failing and it does appear to be related, but I haven't figured what the issue is. Could you add the small example checking both Airtable and AirtableField change correctly. I can then copy it into my test base and we can add it to the travis.yml to have it run against the CI on future PRs so we don't accidentally break it. Maybe in that process we will route out there error.

README.md Outdated
@@ -34,6 +34,7 @@ plugins: [
tableName: `YOUR_TABLE_NAME`,
tableView: `YOUR_TABLE_VIEW_NAME`, // optional
queryName: `OPTIONAL_NAME_TO_IDENTIFY_TABLE`, // optional
createSeparateNodeType: true, // optional - makes all records in this table a separate node type, based on your tableView, or if not present, tableName, e.g. a table called "Fruit" would become "allAirtableFruit"
Copy link
Owner

Choose a reason for hiding this comment

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

Might be worth mentioning that the default is false. Also, we should probably add another section in the docs on why you would care to do this. We could even point back to the issues regarding it.

gatsby-node.js Outdated
@@ -146,6 +148,10 @@ exports.sourceNodes = async (
store,
cache
});

const nodeType = (row.separateNodeType) ?
Copy link
Owner

Choose a reason for hiding this comment

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

The only issue I can think of is that that isn't falsey when we are expecting it to be (in regards to the failing test). Could it be an issue with an unresolved promise?

@jbolda
Copy link
Owner

jbolda commented May 1, 2019

Oh, and let's do a minor bump on this. Feel free to include that in this PR.

@kevee
Copy link
Contributor Author

kevee commented May 3, 2019

Will do! We just did a big launch so I'll probably not get to it until next week.

@jbolda
Copy link
Owner

jbolda commented May 31, 2019

Just checking in to make sure you are not waiting on a review.

@jbolda
Copy link
Owner

jbolda commented Jun 27, 2019

Checking in on this again. Still busy with that launch?

@ndrabins
Copy link

ndrabins commented Nov 7, 2019

Any chance this is still possible to add in? Would love to be able to separate tables by node name.

I'd be willing to take this on if no one else is intereseted. Thanks

@jbolda
Copy link
Owner

jbolda commented Nov 7, 2019

I would love to see this get in @ndrabins. We can merge this PR into a sperate feature branch and you can work off that. Otherwise you can start fresh if that seems to best course of action.

@ndrabins
Copy link

ndrabins commented Nov 9, 2019

Awesome, sounds good @jbolda. I should be able to get to this either this weekend or next.

@ndrabins
Copy link

@jbolda hey! So I tried working on this PR this weekend starting from here. Immediately hit a roadblock in that even when I npm link I get an error about not being able to find the plugin. Any ideas?

Also I haven't tested it yet, but this PR seems pretty solid to my beginner eyes. Was there something specific the original author was missing or just a broken test?

@jbolda
Copy link
Owner

jbolda commented Nov 25, 2019

@ndrabins I think it is pretty close. We needed to get the existing tests to pass and add a new test to check this implementation.

Hmm, that is pretty odd. We are working through an issue on the newest gatsby via #117 that may be related?

Since it sounds like you are in agreement that what is here is good so far, I will go ahead and merge this into a separate feature branch sometime today that we can work on. Sound good to you?

@ndrabins
Copy link

Yea that sounds good. I'll check that out then and go from there. Will try again to see if I can get the linking to work. Thanks!

@jbolda jbolda changed the base branch from master to feature/multi-node-types November 26, 2019 04:18
@jbolda jbolda merged commit cc08b80 into jbolda:feature/multi-node-types Nov 26, 2019
jbolda added a commit that referenced this pull request Feb 15, 2020
* Option to make tables separate node types (#52)

* Option to make tables separate node types

* Revert "Option to make tables separate node types"

This reverts commit b59ffda.

* Removed package/lock from branch

* Added AirtableField node types.

* Added more info for createSeparateNodeType

* Version bump

* Check that separateNodeType option exists

* add option to set mapping fields each as a separate type (#115)

* separateMapTypes config option

* update version

* Feat/multi node types/parent node field (#144)

* prettier all the things

* add separateNodeType option

* spelling mistake

* add separateNodeType

* add example that errors without new features

* bump version number

Co-authored-by: Kevin Miller <keveemiller@gmail.com>
@psmedia psmedia mentioned this pull request Mar 28, 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

Successfully merging this pull request may close these issues.

3 participants