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

[Feature] Remove rest using a future flag #1701

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

byrichardpowell
Copy link
Contributor

@byrichardpowell byrichardpowell commented Oct 29, 2024

WHY are these changes introduced?

Shopify is all-in on graphql. By adding this future flag now we can:

  1. Steer new apps towards GraphQL (We'll enable it by default in the app template).
  2. Give existing apps some time to switch to GraphQL and opt-in to the future flag before a breaking change.

WHAT is this pull request doing?

  1. Add a v4_removeRest future flag: It is false by default. When it is true methods for interacting with Rest will never be in the admin context object. This affects a lot of methods, so there are a lot of changes. Most of hich come from maintaining type safety.
  2. Update existing tests: Mostly this is about type safety. None of these tests should have actually broken, but the tests don't understand that v4_removeRest will be false by default. So a lot of these changes are to do with that.
  3. Add new tests: Mostly this is about updating expectAdminApiClient to receive an admin object that should not cntain anything o do with REST. Then each test that uses expectAdminApiClient is updated to pass an admin object where v4_removeRest is true and an admin object where v4_removeRest is false.
  4. Update docs: A few of the docs use REST examples to illustrate other concepts. So I've changed these to use GraphQL. I also added some context to various guides explaining that REST is deprecated, and linking to the blog post. I added docs for the new future flag and I added a note about future changes.
  5. Mark REST as deprecated: I marked admin.rest as deprecated, with a link to the blog post.

Tophatting

The most important questions are:

  1. Have any return values or types changed when v4_removeRest is undefined. There should be no changes.
  2. Have any return values or types changed when v4_removeRest is false. There should be no changes.
  3. When v4_removeRest is true is REST returned from any of the methods? It should not be returned. The types should match.

Future note

Because of the complexity of our types, the easiest way to remove REST in v4 is probably to revert this PR, then remove REST. If we just manually revert all these changes I worry we'll end up with some complexity in our tests and types that does not need to be there.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have used pnpm changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

@byrichardpowell byrichardpowell force-pushed the remove-rest-from-remix-future-flag branch from b2b2377 to 5f03af3 Compare October 29, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant