-
Notifications
You must be signed in to change notification settings - Fork 27
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
chore: ugrade to apollo v3.4 #2331
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/Hc8tvXan4fwQYSWuWJu2Es8TmR9r |
🦋 Changeset detectedLatest commit: b8a7c03 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -159,6 +159,7 @@ module.exports = function createWebpackConfigForDevelopment(options = {}) { | |||
// Makes some environment variables available to the JS code, for example: | |||
// if (process.env.NODE_ENV === 'development') { ... }. | |||
new webpack.DefinePlugin({ | |||
__DEV__: 'true', |
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.
packages/application-shell/src/hooks/apollo-hooks/apollo-hooks.ts
Outdated
Show resolved
Hide resolved
@@ -27,7 +27,9 @@ export type MenuLoaderResult<Key extends MenuKey> = Key extends 'appBar' | |||
: never; | |||
export type Config<Key extends MenuKey> = { | |||
environment: TApplicationContext<{}>['environment']; | |||
queryOptions?: QueryFunctionOptions; |
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.
Somehow TS was getting a bit confused, so I decided to be explicit in which query options are being used from our side.
// NOTE: we lazily execute the query to ensure that (for development) we can | ||
// write the data into the cache BEFORE the query attempts to read from it. | ||
// If not, Apollo throws an error like `Can't find field 'applicationMenu' on ROOT_QUERY object`. | ||
const [executeQuery, { data: menuQueryResult, called }] = useMcLazyQuery< |
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.
This is due to the changes in Apollo v3.4
fetchPolicy: config.environment.servedByProxy | ||
? queryOptions.fetchPolicy || 'cache-first' |
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.
The queryOptions.fetchPolicy
wasn't really used.
expect(console.error).toHaveBeenCalledWith( | ||
expect.stringContaining(`Missing field 'id'`) | ||
); |
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.
Apollo now also logs an error
Merging, as it's blocking other PR updates. |
Extracted ugrading changes related to Apollo
v3.4
from #2318This Apollo version seems to be a bit buggy, so we wait a bit to see if some of the issues are resolved.