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

schema-reporting: Options renaming + remove experimental_ prefix #4236

Merged

Conversation

sachindshinde
Copy link
Contributor

@sachindshinde sachindshinde commented Jun 11, 2020

This PR removes the experimental_ prefix from schema reporting option names, and specifically changes experimental_schemaReporting to reportSchema. Note that the old option names are still there, but they've been marked @deprecated and trying to use them will log a warning.

CHANGELOG.md Outdated
@@ -12,6 +12,9 @@ The version headers in this history reflect the versions of Apollo Server itself
- `apollo-engine-reporting`: Add environment variable `APOLLO_SCHEMA_REPORTING` that can enable schema reporting. If `experimental__schemaReporting` is set it will override the environment variable. [PR #4206](https://github.com/apollographql/apollo-server/pull/4206)
- `apollo-engine-reporting`: The schema reporting URL has been changed to use the new dedicated sub-domain `https://edge-server-reporting.api.apollographql.com`. [PR #4232](https://github.com/apollographql/apollo-server/pull/4232)
- `apollo-server-core`: Though Apollo Server **is not affected** due to the way it is integrated, in response to [an upstream security advisory for GraphQL Playground](https://github.com/prisma-labs/graphql-playground/security/advisories/GHSA-4852-vrh7-28rf) we have published [the same patch](https://github.com/prisma-labs/graphql-playground/commit/bf1883db538c97b076801a60677733816cb3cfb7) on our `@apollographql/graphql-playground-html` fork and bumped Apollo Server to use it. Again, this was done out of an **abundance of caution** since the way that Apollo Server utilizes `renderPlaygroundPage` is _not_ vulnerable as it does not allow per-request Playground configuration that could allow interpolation of user-input. [PR #4231](https://github.com/apollographql/apollo-server/pull/4231)
- `apollo-engine-reporting`: Change the `experimental_schemaReporting` option name to `experimental_reportSchema`. [PR #4236](https://github.com/apollographql/apollo-server/pull/4236)
Copy link
Member

Choose a reason for hiding this comment

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

Surely this shouldn't contain experimental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the confusion, I made two changelog entries here when really I should have just condensed them into one.

@sachindshinde sachindshinde force-pushed the sachin/remove-experimental-from-schema-reporting branch 2 times, most recently from 2e88316 to 8194e87 Compare June 12, 2020 16:37
@sachindshinde
Copy link
Contributor Author

(I'll update this PR once #4244 and #4246 merge)

Copy link
Contributor

@jsegaran jsegaran left a comment

Choose a reason for hiding this comment

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

I think instead of whole sale removing the experimental_ fields we should deprecate them. In the case someone is using JS they will not get any feedback from their code/build processes that the experimental fields are removed, and we will break the build for anyone using ts.

@jsegaran
Copy link
Contributor

I also think you will want to add a line to docs/source/api/apollo-server.md It looks like I didn't add one for the experimental flags. i think we can just add the new fields?

@sachindshinde sachindshinde force-pushed the sachin/remove-experimental-from-schema-reporting branch from 8194e87 to 814b710 Compare June 15, 2020 21:16
Copy link
Contributor

@jsegaran jsegaran left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think we probably need to wait until 2.15.0 to merge this in? cc @abernix

@abernix abernix changed the base branch from master to release-2.15.0 June 16, 2020 12:33
@abernix
Copy link
Member

abernix commented Jun 16, 2020

I've changed the target branch to release-2.15.0. We can cut an alpha of this from that release branch before the final version is published.

@jsegaran jsegaran merged commit 4c7fcaf into release-2.15.0 Jun 16, 2020
@abernix abernix added this to the Release 2.15.0 milestone Jun 16, 2020
@abernix abernix deleted the sachin/remove-experimental-from-schema-reporting branch February 5, 2021 06:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants