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

Use UUID version 5 for node IDs in source/transformer plugins #1853

Closed
KyleAMathews opened this issue Aug 18, 2017 · 14 comments

Comments

Projects
5 participants
@KyleAMathews
Copy link
Contributor

commented Aug 18, 2017

This was pointed out on Discord https://www.npmjs.com/package/uuid#version-5

Node IDs should be a) globally unique and b) meaningless so people don't rely on them for anything. Standardizing on UUID version 5 seems like a good way to accomplish this.

Thoughts?

@jquense

This comment has been minimized.

Copy link
Collaborator

commented Aug 18, 2017

I think we do some amount of regex'ing off the id as a path for documentation on www. but overall I think it's a good idea :)

@KyleAMathews

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2017

Who wrote that code??? They should be fired! ;-)

Yeah, though we should probably make this change as part of v2.

@jquense

This comment has been minimized.

Copy link
Collaborator

commented Aug 18, 2017

it should already be fine to do deep filtering right e.g. internal.path, I find that most of the time I want the ability to search by file path, maybe we should add it automatically where it makes sense?

@KyleAMathews KyleAMathews referenced this issue Aug 23, 2017

Closed

v2 umbrella issue #1824

5 of 14 tasks complete
@arlair

This comment has been minimized.

Copy link

commented Oct 26, 2017

Hi, I am not sure if I am doing this wrong, but I found it quite useful to be able to specify my own id for a source plugin that used relational database tables. I could make the id based on the table name, row id and a name for my plugin (perhaps with an instance number if it was reusable). Then I don't need to create any hash maps to keep track of the ___NODE relations in the plugin and they can be generated based on the data.

It is also easier to debug the id with an error message something like:

Error: Invariant Violation: Encountered an error trying to infer a GraphQL type for: "data.{tableName}___NODE". There is no corresponding node with the id field matching: "{row id}-{plugin-name}{table name}"

@jquense jquense referenced this issue Oct 26, 2017

Closed

v2 Umbrella issue #2641

7 of 13 tasks complete
@KyleAMathews

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2017

@arlair could you create an issue for the error message? Sounds like a really helpful improvement!

@arlair

This comment has been minimized.

Copy link

commented Oct 26, 2017

@KyleAMathews The error already exists :) I was just showing how useful it became when I was able to specify a custom Gatsby id based on what should be unique information, rather than a new generated UUID id that would be harder to identify.

One option might be to build into the API a postfix for the Gatsby Id based on the plugin name and instance and maybe node type? When I first started trying to build a Gatsby plugin, I assumed I was meant to be passing in my own id and it would make it unique based on my plugin instance and the node type in internal.

I think I found it confusing because what turned out to be the Gatsby id was called id and lived in the root of the Node interface and not the internal area, but I've been meaning to write something up separately about that.

@calcsam calcsam moved this from To Do to In progress in Gatsby v2 Release Jan 11, 2018

@calcsam calcsam moved this from In progress to To Do in Gatsby v2 Release Jan 11, 2018

@danielfarrell

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2018

I'm taking a go at this one, just FYI.

@KyleAMathews

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2018

Thanks @danielfarrell!

@KyleAMathews

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2018

Also sent you an invite to Gatsby's core team so you can assign yourself to issues in the future :-)

@danielfarrell danielfarrell self-assigned this Jan 13, 2018

@danielfarrell danielfarrell moved this from To Do to In progress in Gatsby v2 Release Jan 13, 2018

@calcsam

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Fixed in the PR, closing this

@calcsam calcsam closed this Jan 26, 2018

Gatsby v2 Release automation moved this from In progress to Done Jan 26, 2018

@KyleAMathews KyleAMathews reopened this Jan 26, 2018

@KyleAMathews

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

@calcsam @danielfarrell's PR was just to add the new helper function to v1 so this issue isn't done yet as we need to convert source/transformer plugins in v2 over to use UUIDs for ids.

@danielfarrell

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2018

Is there a process that changes flow from master to v2? I can finish this up for v2, but didn't want to duplicate that code if it would flow through.

@KyleAMathews

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

Yeah, I merge them in :-) Lemme do that real quick.

@KyleAMathews

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

merged

@danielfarrell danielfarrell moved this from Done to In progress in Gatsby v2 Release Feb 1, 2018

@KyleAMathews KyleAMathews moved this from In progress to Done in Gatsby v2 Release Feb 10, 2018

@ghost ghost removed the review label Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.