-
Notifications
You must be signed in to change notification settings - Fork 685
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
Upgrade to @apollo/client@3 #2560
Conversation
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
If your PR is missing information, check against the original template here. At a minimum you must have the section headers from the template and provide some information in each section. |
Signed-off-by: sirugh <rugh@adobe.com>
13453c3
to
91c1f3d
Compare
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Performance Test Results The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate. https://pr-2560.pwa-venia.com : LH Performance Expected 0.85 Actual 0.35, LH Accessibility Expected 1 Actual 0.97, LH Best Practices Expected 1 Actual 0.93, WPT Cache Expected 90 Actual 35.666666666667 |
Signed-off-by: sirugh <rugh@adobe.com>
ae98fc7
to
d5f4167
Compare
packages/peregrine/lib/talons/CartPage/PriceAdjustments/ShippingMethods/useShippingMethods.js
Show resolved
Hide resolved
@@ -18,7 +18,7 @@ export const useCartPage = props => { | |||
|
|||
const { called, data, loading } = useQuery(getCartDetails, { | |||
fetchPolicy: 'cache-and-network', | |||
// Don't make this call if we don't have a cartId | |||
nextFetchPolicy: '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.
This answers one of my previous questions, thanks for the context 👍
@@ -1,5 +1,5 @@ | |||
const AbstractCompiledResource = require('./AbstractCompiledResource'); | |||
const gql = require('graphql-tag'); | |||
const gql = require('@apollo/client'); |
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.
Interesting that they chose to make gql
the default export of @apollo/client
.
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.
After looking at the other imports, this one needs to be changed.
import { gql } from '@apollo/client';
But actually, I don't even know what this file is. Is this generated code?
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.
Honestly, not sure if this file is even used at all. I'll poke around, but yes this does need to be changed to at least:
const apolloClient = require('apollo/client');
const { gql } = apolloClient;
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.
read(cached) { | ||
return cached || null; | ||
} | ||
} |
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 actually pretty readable
introspectionQueryResultData: UNION_AND_INTERFACE_TYPES | ||
}) | ||
typePolicies, | ||
possibleTypes: POSSIBLE_TYPES |
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.
May want to drop a comment where POSSIBLE_TYPES
comes from since it's not otherwise present in this file
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.
Done 8eafc91
Query: { | ||
fields: { | ||
cart: { | ||
// Replaces @connection(key: "Cart") |
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.
Makes me think we should update our Mutation PII Cheat Sheet.
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.
Not sure if the use of the policy for the Cart
field warrants the update, but I think I get where you're going with this. We could preempt this and move all the connection directives from mutations to this policies file. For example this mutation:
export const REMOVE_GIFT_CARD_MUTATION = gql`
mutation removeGiftCard($cartId: String!, $giftCardCode: String!) {
removeGiftCardFromCart(
input: { cart_id: $cartId, gift_card_code: $giftCardCode }
) @connection(key: "removeGiftCardFromCart") {
...
}
}
`;
would become
{
Mutation: {
fields: {
removeGiftCardFromCart: {
keyArgs: () => 'removeGiftCardFromCart'
}
}
}
}
Then the rule in the cheatsheet, and something we'd have to enforce in reviews, would be to add a field keyArg
rule to the policies file when adding a new mutation.
I'd like to get opinions on that though.
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.
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.
Thoughts:
-
Have each component use the
useTypePolicies
hook to add type policy keys for their mutations at runtime. This would prevent initial bundle bloat and aid in chunking. -
Declare all mutation keys up front. Adds bytes to bundle and hurts chunking if we aren't using all mutations.
note: We could generate this automatically from the schema if we knew what fields we consider "pii". -
Use a global
no-cache
policy for mutations. This would remove the need to do any of this, but would force us to refactor our app because we use mutation results to auto update cache. We would have to switch to usingrefetchQueries
after each mutation.
/** | ||
* @deprecated Resolvers are deprecated in ApolloClient v3. |
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.
I never know where to draw that line.
- This is going to be a major change anyway, right?
- When do we actually remove this code?
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
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 good from my end. There are still minor things to resolve, but I'm good when others' feedback is resolved.
/** | ||
* @deprecated Resolvers are deprecated in ApolloClient v3. |
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.
- Yes, it's absolutely going to be a major change.
- We haven't actually formalized a policy for removal, only deprecation.
I think the pacing of our major releases is a bit too fast for us to change code from "canonical" to "deprecated" to "removed" just like that. Now that we're properly marking things with @deprecated
, we can start the conversation around an actual removal timeframe.
Signed-off-by: sirugh <rugh@adobe.com>
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.
Nice improvements with making type policies composable, this looks ready for QA 👍
* Copy frozen array before sorting. * Upgrade to Apollo v3 - imports and package.json Signed-off-by: sirugh <rugh@adobe.com> * Fixup some frozen object and array modifications Signed-off-by: sirugh <rugh@adobe.com> * Replaces cacheKeyFromType Signed-off-by: sirugh <rugh@adobe.com> * Remove apollo/client from direct peregrine dependency Signed-off-by: sirugh <rugh@adobe.com> * prettier Signed-off-by: sirugh <rugh@adobe.com> * Upgrade to v3.0.2 Signed-off-by: sirugh <rugh@adobe.com> * Add more type policies for cart fields * Always use incoming items as result Signed-off-by: sirugh <rugh@adobe.com> * Require url for ProductImage key Signed-off-by: sirugh <rugh@adobe.com> * Fixup typePolicies Signed-off-by: sirugh <rugh@adobe.com> * Replace cart connection key with keyArgs Signed-off-by: sirugh <rugh@adobe.com> * Use constants for custom key fields Signed-off-by: sirugh <rugh@adobe.com> * Ignore CartPrices id Signed-off-by: sirugh <rugh@adobe.com> * Trying to perfect usage of type policy Signed-off-by: sirugh <rugh@adobe.com> * Update mutation link version and ensure 3.0.2 client is used Signed-off-by: sirugh <rugh@adobe.com> * Use a skippable query hook as a stop gap while skip is fixed in apolloclient core Signed-off-by: sirugh <rugh@adobe.com> * Revert "Use a skippable query hook as a stop gap while skip is fixed in apolloclient core" This reverts commit 832f6f4. * Upgrade to apolloclient v3.1.2 Signed-off-by: sirugh <rugh@adobe.com> * Undo pagebuilder package auto format * Re add updated product cache hit logic. * use client import Signed-off-by: sirugh <rugh@adobe.com> * Move price query under correct parent * Fall back to use product id when no url_key * Protect against undefined cachedata * Ensure currency is fetched on sign in to properly merge carts Signed-off-by: sirugh <rugh@adobe.com> * ProductSearch must prefetch just like category page does otherwise the pdp will crash from missing cache data Signed-off-by: sirugh <rugh@adobe.com> * Use apollo client import for new communications page Signed-off-by: sirugh <rugh@adobe.com> * Dont throw if fragment is missing all data from cache Signed-off-by: sirugh <rugh@adobe.com> * Revert "ProductSearch must prefetch just like category" This reverts commit fef660f. * revert package indentation Signed-off-by: sirugh <rugh@adobe.com> * only try to merge incoming if exists Signed-off-by: sirugh <rugh@adobe.com> * Merge cart shipping addresses using index within address array. This is not perfect but the alternative is to overfetch since cache data would always be incoming. Signed-off-by: sirugh <rugh@adobe.com> * Use nocache in lazy query definition otherwise it caches. Removes unnecessary code fetch from fragment Signed-off-by: sirugh <rugh@adobe.com> * Remove unused connection key from cart query Signed-off-by: sirugh <rugh@adobe.com> * use nextFetchPolicy of cache-first to prevent network request when cache data changes Signed-off-by: sirugh <rugh@adobe.com> * Rename gift cards query to make similar to applied coupons Signed-off-by: sirugh <rugh@adobe.com> * Handle addition and removal of gift cards in client merge function Signed-off-by: sirugh <rugh@adobe.com> * Rename stuff to prepare function for moving to util Signed-off-by: sirugh <rugh@adobe.com> * Rename react-hooks imports Signed-off-by: sirugh <rugh@adobe.com> * Missed import Signed-off-by: sirugh <rugh@adobe.com> * Refactor deprecated gift_option resolvers * Add missing stock status to items review fragment Signed-off-by: sirugh <rugh@adobe.com> * Remove deprecated payment information resolvers. Signed-off-by: sirugh <rugh@adobe.com> * Use no-cache policy to avoid requery on cache changes Signed-off-by: sirugh <rugh@adobe.com> * Add merge for CustomerAddress.street as value is in array Signed-off-by: sirugh <rugh@adobe.com> * Readd graphql resolution as upgrading to 15 or removing it entirely causes validator to fail Signed-off-by: sirugh <rugh@adobe.com> * Fixup gql validation errors Signed-off-by: sirugh <rugh@adobe.com> * Fix incorrect boolean response from type policy Signed-off-by: sirugh <rugh@adobe.com> * Removes unused/invalid import Co-authored-by: Jimmy Sanford <jimbo@users.noreply.github.com> * Cast errors to boolean Co-authored-by: Jimmy Sanford <jimbo@users.noreply.github.com> * Applied coupons is an array with single value or null, so we don't need the merge function Signed-off-by: sirugh <rugh@adobe.com> * Add useTypePolicies hook and use in product talon * Only show indicator if loading and no data * Simplify read fn Signed-off-by: sirugh <rugh@adobe.com> * Update mapProduct docs * Add docs for getPossibleTypes Signed-off-by: sirugh <rugh@adobe.com> * Revert currency fetch where unnecessary. Add comment explaining what happens if currency value is not fetched in merge mutation. * Coupon Code uses skip instead of lazy query Signed-off-by: sirugh <rugh@adobe.com> * Use merge:true, same thing anyways Signed-off-by: sirugh <rugh@adobe.com> * Removes prefetch optimizations and url_key identifier use that resulted in cache misses. We found that url_key is not unique so it cannot be used as product interface id Signed-off-by: sirugh <rugh@adobe.com> * Move type policies and merge functions into separate files Signed-off-by: sirugh <rugh@adobe.com> * add line breaks and comments Signed-off-by: sirugh <rugh@adobe.com> * Always use incoming values when merging gift cards and shipping addresses * Update comments. * Feedback Signed-off-by: sirugh <rugh@adobe.com> * Fixup tests Signed-off-by: sirugh <rugh@adobe.com> * Update jsdoc for useTypePolicies Signed-off-by: sirugh <rugh@adobe.com> * use apollo/client instead of react-hooks Signed-off-by: sirugh <rugh@adobe.com> Co-authored-by: Jimmy Sanford <jimbo@users.noreply.github.com> Co-authored-by: Devagouda <40405790+dpatil-magento@users.noreply.github.com>
https://www.apollographql.com/docs/react/migrating/apollo-client-3-migration/
Notes from upgrade
read
functions replace local resolversmerge
functions handle ambiguous normalization (ie. arrays of data, objects with no keys)TODO:
Migrate gift option resolvers to type policiesDoneMigrate payment information resolvers to type policiesDoneConvert to possibleTypes from- DonedataFromObject
Fix PDP Overfetching- Resolved by removing manual cache lookups in useProductFix Cart Overfetching (and/or blowing away old data ie region/country).Could not repro anymore.Release/Update apollo-link-mutation-queue sinceDone.operation.toKey
is no longer a function.Handle- AC@3.1.2 resolved this.skip
bug.graphiql schema view breaks nowThis appears broken on develop.