-
Notifications
You must be signed in to change notification settings - Fork 14
Rework getting started pages #273
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
Conversation
Alternatively, if you already have data in the AuraDB, you can obtain your type definitions via the https://graphql-toolbox.neo4j.io[Neo4j GraphQL Toolbox]. | ||
The toolbox can connect to the AuraDB and automatically create type definitions and allow GraphQL operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note we want to have the introspection feature in the Aura console soon, so this will need to be updated to reflect that when it's added.
Co-authored-by: Michael Webb <[email protected]>
|
||
[source, javascript, indent=0] | ||
---- | ||
const driver = neo4j.driver( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I love that we're hardcoding the credentials here without a note that it's not best practice (anyone with access to your source code now has access to your DB).
Either we just add a warning/note about this risk or we should try to use environment variables e.g.
const {
NEO4J_URI = "neo4j://localhost:7687/neo4j",
NEO4J_USERNAME = "neo4j",
NEO4J_PASSWORD = "password",
} = process.env;
const driver = neo4j.driver(
NEO4J_URI,
neo4j.auth.basic(NEO4J_USERNAME, NEO4J_PASSWORD)
);
With this example it's still possible to hardcode to get going quickly but we should prompt the user to not commit real creds and set them using environment variables going forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you have any thoughts on this or better ideas @mjfwebb?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of pointing towards using process.env, though I am also hesitant at implying it's "the secure way of doing things" because that really depends on the rest of the setup.
Keeping it hard-coded might be easiest so long as we explicitly state we do not recommend keeping it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a note then
the point of the tutorial is to have a minimal working example to get started - let's keep the bar low and the code examples short/simple
|
||
To learn more, keep reading the documentation about xref:queries-aggregations/index.adoc[Queries and aggregations] or alternatively learn how to use the xref:getting-started/toolbox.adoc[Neo4j GraphQL Toolbox]. | ||
For more advanced database settings, refer to the xref:driver-configuration.adoc[Driver configuration] page. | ||
These pages walks you through creating a new project with the Neo4j GraphQL Library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These guides are both based on the northwind dataset right?
Is it worth mentioning here or maybe at the end of the guides that as a next step users could try loading that dataset into their db and extending the schema for the full data set to see what that adds to their api?
Ideally we could also write a guide for this in the future too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i had added a reference to the northwind dataset initially, but the examples work without it so far (or rather, only work when no other data is present). the idea was to go in that direction and make it the default data set for all examples
i'd say, we leave it out for now, and revisit when the picture is more coherent. maybe we could add a page that is specifically about importing and then making the northwind set available to a client via the api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can leave for now then. Tbf, I think we'll be able to get to a tutorial for a more complex schema soon anyway and that would be a good time to look at this anyway
Co-authored-by: Liam-Doodson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good and is a big improvement!
I've just added one more small comment and would still like to see a note/warning about not hard coding credentials
Other than that though this LGMT
I'd also hold off on merging until @MacondoExpress & @mjfwebb have had a chance to review too though
This tutorial shows you how to create and use a GraphQL Data API in Aura Console. | ||
|
||
|
||
== Create a GraphQL Data API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding a prerequisite that you need to have already created an AuraDB and linking to the docs on how to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely - in fact, we have a prerequisites section in the self-hosted page now, the aura page should mirror that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a small typo
This PR includes documentation updates New pages: Updated pages: |
No description provided.