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.

Sign up for GitHub

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

Jump to bottom

Graphql integration #525

Merged
merged 8 commits into from
Oct 5, 2023
Merged

Graphql integration #525

merged 8 commits into from
Oct 5, 2023

Conversation

louiszawadzki
Copy link
Contributor

@louiszawadzki louiszawadzki commented Sep 12, 2023 •
edited
Loading

What does this PR do?

Add integration to collect GraphQL queries and mutations.

I've checked with proxies that the correct information was sent in the payloads.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
  • If this PR is auto-generated, please make sure also to manually update the code related to the change

@louiszawadzki louiszawadzki force-pushed the louiszawadzki/graphql-integration branch 2 times, most recently from d91c253 to 0f2a179 Compare September 20, 2023 09:17
@louiszawadzki louiszawadzki force-pushed the louiszawadzki/graphql-integration branch 3 times, most recently from f08502c to 3c103a4 Compare September 22, 2023 08:37
@louiszawadzki louiszawadzki force-pushed the louiszawadzki/graphql-integration branch 2 times, most recently from dad2621 to c12870d Compare September 22, 2023 12:13
@louiszawadzki louiszawadzki marked this pull request as ready for review September 22, 2023 12:13
@louiszawadzki louiszawadzki requested review from a team as code owners September 22, 2023 12:13
estherk15
estherk15 previously approved these changes Sep 22, 2023
View reviewed changes
packages/react-native-apollo-client/README.md Outdated Show resolved Hide resolved
packages/react-native-apollo-client/README.md Outdated Show resolved Hide resolved
packages/react-native-apollo-client/README.md Outdated Show resolved Hide resolved
maciejburda

This comment was marked as outdated.

Sign in to view

@louiszawadzki louiszawadzki force-pushed the louiszawadzki/graphql-integration branch from 919aa2f to d04ddb7 Compare September 25, 2023 08:32
@louiszawadzki louiszawadzki force-pushed the louiszawadzki/graphql-integration branch from d04ddb7 to 5dfbf0c Compare September 25, 2023 08:41
estherk15
estherk15 previously approved these changes Sep 25, 2023
View reviewed changes
Copy link

@estherk15 estherk15 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, left one optional recommendation!

packages/react-native-apollo-client/README.md Outdated
};
```

## Access your GraphQL resources information
Copy link

Choose a reason for hiding this comment

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

Do you anticipate customers asking about this? Recommend removing this section.

Copy link
Contributor Author

@louiszawadzki louiszawadzki Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right now adding the GraphQL integration will change strictly nothing for users as the web platform does not display the information yet.

The goal is to warn users that this is not usable yet in case they came across this package so they don't try it out and feel let down.

I can replace this by a warning at the top of the README if that makes the purpose clearer?

Copy link

Choose a reason for hiding this comment

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

@louiszawadzki If this is a feature that we plan on supporting in the future, an alert it might work better. Not sure if the alert div is supported in this repo, but this is what we usually add to highlight information in docs:

Suggested change
## Access your GraphQL resources information
<div class="alert alert-info">Adding the GraphQL integration does not change any existing features. The information is not surfaced on the Datadog platform.</div>

0xnm
0xnm previously approved these changes Sep 26, 2023
View reviewed changes
Copy link
Member

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

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

nice work!

packages/react-native-apollo-client/src/helpers.ts Outdated
})[0] || null
);
} catch (e) {
// TODO: telemetry
Copy link
Member

Choose a reason for hiding this comment

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

minor: it is better to create a ticket straight away and add its number here

@louiszawadzki louiszawadzki dismissed stale reviews from 0xnm and estherk15 via 10af167 September 27, 2023 09:22
@louiszawadzki louiszawadzki force-pushed the louiszawadzki/graphql-integration branch from 5dfbf0c to 10af167 Compare September 27, 2023 09:22
@louiszawadzki louiszawadzki merged commit 487bab0 into develop Oct 5, 2023
3 checks passed
@louiszawadzki louiszawadzki deleted the louiszawadzki/graphql-integration branch October 5, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xnm 0xnm 0xnm approved these changes

@maciejburda maciejburda Awaiting requested review from maciejburda

@estherk15 estherk15 Awaiting requested review from estherk15

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@louiszawadzki @0xnm @maciejburda @estherk15