Fix stale product options #1210

blittle · 2022-05-05T15:06:01Z

This comes up when you directly navigate from one product to another, and the product options are not the same. See this stackblitz. Notice the related products at the bottom of the details page that allow you to navigate directly to other products. As you do so, the product options get lost.

The bigger issue exposed here is that we don't lint our framework code. I updated this PR to now lint! Because of that, there are a lot of other changes. I commented throughout the PR on more significant things found by the linter.

packages/hydrogen/src/hooks/useProductOptions/useProductOptions.ts

blittle · 2022-05-06T20:48:59Z

packages/hydrogen/src/entry-server.tsx

  ReactApp: JSX.Element,
  {log, nonce}: {log: Logger; nonce?: string}
 ): Promise<string> {
-  return new Promise<string>(async (resolve, reject) => {


This is something caught by the linter that I think we should avoid. Having an async function as the resolver of a Promise. The issue is that uncaught errors within the async function won't bubble to any outer .catch statements

blittle · 2022-05-06T20:49:58Z

packages/hydrogen/src/foundation/Router/BrowserRouter.client.tsx

  children,
 }) => {
  if (META_ENV_SSR) return <>{children}</>;
+  /* eslint-disable react-hooks/rules-of-hooks */


This intentionally runs slightly different in SSR mode

blittle · 2022-05-06T20:51:08Z

packages/hydrogen/src/foundation/useUrl/useUrl.ts

+  // eslint-disable-next-line react-hooks/rules-of-hooks
+  return useMemo(() => new URL(window.location.href), [location]); // eslint-disable-line react-hooks/exhaustive-deps


This memoization is interesting. I assume it is correct. But at the same time I'm wondering if it maybe should be useMemo(() => new URL(location.href), [location]);?

blittle · 2022-05-06T20:51:55Z

packages/hydrogen/src/hooks/useProductOptions/tests/useProductOptions.test.tsx

    });
  });

-  it('computes selected variant based on options', async () => {


This test was essentially doing exactly what the other one was.

blittle · 2022-05-06T20:52:34Z

packages/hydrogen/src/hooks/useProductOptions/useProductOptions.client.ts

-  }, [initialVariantId, variants]);
+    const variant = getSelectedVariant(variants, selectedOptions);
+    setSelectedVariant(variant);
+  }, [selectedOptions, variants]);


I simplified the two effects in here into just one.

frehner

Looks so good!

packages/hydrogen-ui/src/storefront-api-types.d.ts

frehner · 2022-05-06T21:15:10Z

packages/hydrogen/src/components/Link/Link.client.tsx

+        clientState,
+        onClick,
+        location,
+        navigate,


Rules of hooks, yay

frehner · 2022-05-06T21:19:37Z

packages/hydrogen/src/components/ShopPayButton/ShopPayButton.client.tsx

  return (
-    <div className={className} tabIndex={1}>
+    /* eslint-disable jsx-a11y/no-noninteractive-tabindex */
+    <div className={className} tabIndex={0}>


Hm, this is a weird one. I feel like it would actually make more sense to have the tabindex on the custom element, but I don't know enough to know if that's correct or not.

Yeah, I don't know either. I think it should be looked at in a separate PR?

I don't have a lot of context here, but from my observation... Pressing Enter when focused on this div here should have no effect so it shouldn't be tab-able IMO. In other words: the tabIndex should be removed (and the disable comment with it).

Adding tabIndex to the custom component would make sense to me too, but has no effect either when you hit Enter. I think this interaction/event-handling needs to be handled within the script that controls the component (the third-party shop-js/client script).

So I think the solution here is for the custom component to handle its own focus events internally, and we shouldn't have any tabIndex attribute overrides.

frehner · 2022-05-06T21:24:18Z

packages/hydrogen/src/foundation/Redirect/Redirect.client.tsx

    } else {
      navigate(to);
    }
+    // eslint-disable-next-line react-hooks/exhaustive-deps


Any particular reason we want to ignore this one?

I was getting weird issues where it would execute twice, and the page wouldn't redirect. Because it's a redirect, it shouldn't matter though.

frehner · 2022-05-06T21:26:56Z

packages/hydrogen/src/hooks/useProductOptions/useProductOptions.client.ts

-  }, [initialVariantId, variants]);
+    const variant = getSelectedVariant(variants, selectedOptions);
+    setSelectedVariant(variant);
+  }, [selectedOptions, variants]);


caution-tape-bot · 2022-05-09T14:13:57Z

👋 It looks like you're updating JavaScript packages that are known
to cause problems when duplicated.

You can deduplicate them with the yarn-deduplicate utility:

npx yarn-deduplicate --packages graphql react react-dom webpack
npx yarn-deduplicate --scopes @shopify @apollo

If running these commands doesn't produce a change in your yarn.lock file,
you didn't have duplications in these packages and can carry on.

A duplicate React version may cause an invalid hook call warning.

React context providers usually use module-scoped globals as their
default context values. Multiple versions of such packages will yield
multiple global instances, resulting in oblique runtime errors.

blittle · 2022-05-09T14:33:12Z

packages/hydrogen/src/hooks/useMoney/hooks.tsx

-
  const value = useMemo(
-    () => standardCurrencyFormatter.format(amount),
+    () => new Intl.NumberFormat(locale, options).format(amount),


Might as well mov Intl.NumberFormat into the memo 🤘

jplhomer

Thanks for the diligent work!

* v1.x-2022-07: Fix stale product options (#1210) Upgrade body-parser in hydrogen package (#1232) Add new options to Money and useMoney (#1215) fix links (#1229) Update turbo and instructions for developing `dev` (#1225) Heck - deploy all branches to Oxygen add context for initialvariantid (#1217) Build chunks are inside assets folder (#1211) Upgraded to SFAPI 2022-07 (#1214) [ci] release v1.x-2022-07 (#1205) Make this a patch instead of minor add references to video in file_reference block (#1197) Laying the foundation for building components in isolation (#1188) Make metafields optional within the ProductProvider. Fixes #1127 (#1209) Add README to /templates directory (#1163) fix perf tracking and make it optional from developer's end (#1096) docs fixes (#1204)

blittle requested a review from frehner May 5, 2022 15:09

frehner reviewed May 5, 2022

View reviewed changes

packages/hydrogen/src/hooks/useProductOptions/useProductOptions.ts Show resolved Hide resolved

blittle force-pushed the bl-fix-product-options branch from ce43bf8 to b5c9499 Compare May 6, 2022 20:47

blittle commented May 6, 2022

View reviewed changes

github-actions bot deployed to preview May 6, 2022 20:50 View deployment

blittle commented May 6, 2022

View reviewed changes

github-actions bot deployed to preview May 6, 2022 20:56 View deployment

github-actions bot deployed to preview May 6, 2022 21:16 View deployment

frehner reviewed May 6, 2022

View reviewed changes

blittle requested a review from cartogram May 9, 2022 14:02

Shopify deleted a comment from caution-tape-bot bot May 9, 2022

blittle added 4 commits May 9, 2022 10:12

Add eslint back! And fix lots of issues!

8e584cc

Add changeset

cec2aa4

Dedupe deps

9e9f956

Fixes

9e82836

blittle force-pushed the bl-fix-product-options branch from 0340b93 to 9e82836 Compare May 9, 2022 14:13

github-actions bot deployed to preview May 9, 2022 14:15 View deployment

Fix lint issues in the useMoney hook

ecdcab3

blittle commented May 9, 2022

View reviewed changes

github-actions bot deployed to preview May 9, 2022 14:33 View deployment

jplhomer approved these changes May 9, 2022

View reviewed changes

blittle merged commit a844d26 into v1.x-2022-07 May 9, 2022

blittle deleted the bl-fix-product-options branch May 9, 2022 14:47

github-actions bot mentioned this pull request May 9, 2022

[ci] release v1.x-2022-07 #1220

Merged

		// eslint-disable-next-line react-hooks/rules-of-hooks
		return useMemo(() => new URL(window.location.href), [location]); // eslint-disable-line react-hooks/exhaustive-deps

Fix stale product options #1210

Fix stale product options #1210

Uh oh!

Conversation

blittle commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frehner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cartogram May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caution-tape-bot bot commented May 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jplhomer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

blittle commented May 5, 2022 •

edited

Loading

cartogram May 9, 2022 •

edited

Loading