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

DOCS: New nested fields, args for ApolloGraphQLManager #8469

Merged

Conversation

unclecheese
Copy link

@@ -1080,13 +1080,67 @@ Now, let's update the query to fetch our new field.
*my-other-app/client/transformReadNotes.js*
```js
const transformReadNotes = (manager) => {
manager.addField('Priority');
manager.addField('root', 'Priority');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is obviously an API break, and while I know we don't really treat our React API surface with semver yet, this does seem like it would break for anyone who had read these docs in the past and used the example code. Could you switch the argument order to make it B/C?

Copy link
Member

Choose a reason for hiding this comment

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

I've picked up on this in my review as well. Apart from API breakages (which I'm less worried about for those early stage APIs), I think it's too verbose. See silverstripe/silverstripe-admin#683 (comment). We should declare any API breakages on unstable APIs like this clearly in our upgrading notes though.

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

LGTM, just one question about semver compatibility and whether we're concerned about it at this point

@unclecheese unclecheese changed the base branch from 4 to 4.3 October 16, 2018 01:11
@unclecheese unclecheese force-pushed the pulls/4/sense-and-extensibility branch from 99a457c to 3fcbead Compare October 17, 2018 22:23
@robbieaverill robbieaverill merged commit e407882 into silverstripe:4.3 Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants