-
Notifications
You must be signed in to change notification settings - Fork 7
Adding first draft of incremental delivery spec artifact #67
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
base: main
Are you sure you want to change the base?
Conversation
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.
Very cool! Small preference for just incremental
instead of incrementalDelivery
for brevity but very cool to have this versioned 🚀
Also going back to @martinbonnin's comment above - any preference amongst others too? |
@calvincestari @martinbonnin just to make sure I understand, is the I'm indifferent if you're referring to the spec name, so pick your favorite. My only preference on the header value is |
I was referring to both, or either. Shorter is maybe better? |
|
+1 for short parameter
|
OK, I've shortened all the things related to incremental vs. "incremental delivery"; header value, filenames, etc. I think this is now ready for formal review and then merging. |
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.
Might be worth adding an entry in index.md
for better discoverability.
But otherwise very good resource to have around 🤩 , thanks!
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.
Looks great! I'll get the PRs I have open updated 🙂. Thanks!
I've got it in |
Done in 33b4062. |
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.
🎉
What do y'all think about also adding a |
Or might it be an option to specify this as |
@BoD I was wondering the same thing! I'd appreciate that as well. @phryneas I'd rather have the versions reflect only the specs we've implemented. Even if graphql.js has had 3 versions, let's keep the versions either |
Sure, I can try add the implemented legacy spec as |
I don't think we have the same information across both versions:
tl;dr - we're missing opposite pieces of information for each version. |
@calvincestari Maybe for the legacy one we could also use the RFC, but the previous commit, which as far as I can tell corresponds to the format we implemented? |
@BoD that commit definitely looks like the old format so I think you're correct. |
|
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.
Looks great! Appreciate you handling this!
Getting something documented that we can always refer back to.