Algolia hooks: update and patch #700

danielbeardsley · 2022-09-13T21:00:49Z

We have an open pull to make a change in the algolia react-instantsearch repo, but they
likely won't merge it. Since we need this diff to build with the newest version
of the hooks on vercel, let's use pnpm's patch utility to apply the change for now.

When we upgrade to React 18 we just need to drop the .js in the import
statement.

This was made using pnpm's built-in patch commands. Note: pnpm patch
just hung on pnpm v6, so I updated to v7. Seems there were some problems in the past... but they seem to be fixed now?

This also upgrades husky to v8 cause v4 wasn't compatible with pnpm 7

Closes #660

We have an open pull to make a change in this algolia repo, but they likely won't merge it. Since we need this diff to build on vercel, let's just include it directly. When we upgrade to React 18 we just need to drop the `.js` in the import statement. This was made using pnpm's built-in patch commands. Note: `pnpm patch` just hung on pnpm v6, so I updated to v7, ran the commands, then downgraded back to v6.

"pnpm patch" is only available in pnpm 7. Let's give it another try.

husky 4 wasn't compatible with pnpm 7, go figure... Also, allow unmet peer dependencies (this defaults to true for pnpm 7, so we switch it to true to maintain behavior)

vercel · 2022-09-13T21:00:52Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
react-commerce	✅ Ready (Inspect)	Visit Preview	Sep 24, 2022 at 0:50AM (UTC)
react-commerce-prod	✅ Ready (Inspect)	Visit Preview	Sep 24, 2022 at 0:50AM (UTC)

package.json

sterlinghirsh · 2022-09-13T23:58:53Z

CR ⚡ dev_block ⚡ on pinning the husky version, though I'd like it if @federicobadini and/or @dhmacs took a look and made sure this looks reasonable to you too.

sterlinghirsh

Oh I meant to request changes instead of approve.

danielbeardsley · 2022-09-14T00:43:20Z

un_dev_block 👍

I made the changes?

masonmcelvain

CR ✌🏻 Cool! I'll let @federicobadini or @dhmacs approve like @sterlinghirsh suggested

federicobadini

It seems that there is something that needs to be handled differently regarding navigation/reconciliation of the search state.

When we navigate from a filtered collection to a supposedly unfiltered collection we end up with the previously selected filters active.
Here is a video showing the problem:

Screen.Recording.2022-09-15.at.08.45.19.mov

frontend/package.json

masonmcelvain · 2022-09-15T14:40:48Z

This might be unrelated to what @federicobadini discovered, but this is another workaround we added for the url on the first render.

react-commerce/frontend/components/common/InstantSearchProvider.tsx

Lines 151 to 154 in 943eb88

    
           } else if (renderCount === 1 && itemTypeHandle) { 
        
              // Prevents a bug when visiting /Parts/iPhone/Cables 
        
              // that it would redirect to /Parts/iPhone 
        
              path += `/${itemTypeHandle}`;

I wonder if we can drop this else if clause now that we've upgraded.

federicobadini · 2022-09-15T14:50:00Z

This might be unrelated to what @federicobadini discovered, but this is another workaround we added for the url on the first render.

react-commerce/frontend/components/common/InstantSearchProvider.tsx

Lines 151 to 154 in 943eb88

} else if (renderCount === 1 && itemTypeHandle) {

// Prevents a bug when visiting /Parts/iPhone/Cables

// that it would redirect to /Parts/iPhone

path += `/${itemTypeHandle}`;

I wonder if we can drop this else if clause now that we've upgraded.

I did a quick test before and it seemed not present anymore but I have not covered all the possible test cases yet (first render / client side transition / back button navigation etc). I just tried to open the link in the comment.
Definitely it would be good to clean up that part if not needed anymore

danielbeardsley · 2022-09-15T19:46:55Z

I updated the version but I don't have to time to dive deeper right now. Anyone else is welcome to pick this up.

…-and-patch Conflicts: pnpm-lock.yaml * Lots of conflicts, but our branch and more significant changes (and a version bump of the format). So I kept our version

Master had added one dep since we diverged, so I kept our version of the lock file then did `pnpm install` to ensure it was up to date with with package.json requirements.

danielbeardsley · 2022-09-15T19:56:35Z

Fixed the conflicts.

We validated that qsModule.parse() already does url decoding and we don't need to re-do it on the return values. Also, drop some defaults on destructuring because they already happen in the code below.

This was added a bit ago because the server side state for algolia wasn't matching up (or wasn't available). See: a5481b5 This concern has been addressed and this code is now *causing* a serverside / clientside state mismatch and react is complaining that the two states don't match. Removing this and depending on algolias knowledge of the refinements produces relaible results.

corepack was saying it can't find any version compatible with '7' I still don't understand why corepack helps us here, so I'm reverting to plain 'npm install' which is simpler and actually let's us install the latest version.

danielbeardsley · 2022-09-21T20:08:30Z

Ok, @masonmcelvain and I have addressed the issues he brought up, so I think we are good.

un_dev_block 👍

This field is unused as of 4ec07e2, so let's drop it to prevent accidental usage in the future.

sterlinghirsh · 2022-09-21T21:58:13Z

CR ⚡ dev_block ⚡ on what looks like relevant test failures. I think we put the deviceItemType stuff in the findProductList function in part to make sure that the canonical on item type pages included that. It's possible that didn't work before because of the Algolia bugs fixed by the new hooks version. I don't remember if we added a test for canonicals on item type pages, but it's worth a check.

danielbeardsley · 2022-09-24T00:37:24Z

I think the failures were a quirk of having installed pnpm through both corepack and npm. Notice it only failed on one of them. Either way, I think we don't need to dev block on a CI failure (the CI failure already blocks).

un_dev_block 👍 I've re-run the failed build.

Previous one was failing for some mysterious reasons.

masonmcelvain

CR ✌🏻

erinemay · 2022-09-26T16:04:11Z

QA 🎦

Applying multiple filters and moving down the tree with model buttons and moving up the tree with the breadcrumbs. No filters were preserved with upward or downward navigation (both filters and item type facets)
Using the back button with filters, item type facets, search. Back button still worked as expected.
Using the back button to navigate back to a page with multiple filters (all were present)
Entering a page directly (refreshing) with and without facets
Checked with prod preview next to ifixit.com/Parts and confirmed the same above filter behavior on both
Search results and behavior when entering or deleting text is identical between prod-preview and ifixit.com (checked various search terms with identical filters)
Everything in Algolia hooks: update and patch #700 (comment) is the same
Can't reproduce Algolia hooks: update and patch #700 (review)
Can't reproduce Algolia hooks: update and patch #700 (comment)
Can't reproduce Algolia hooks: update and patch #700 (review)

federicobadini and others added 5 commits September 9, 2022 16:19

feat: update instantsearch hooks

78f126a

pnpm: update lock file after installing

de4e706

pnpm: update to v7

2a1246d

"pnpm patch" is only available in pnpm 7. Let's give it another try.

husky: upgrade to 8

e2e9427

husky 4 wasn't compatible with pnpm 7, go figure... Also, allow unmet peer dependencies (this defaults to true for pnpm 7, so we switch it to true to maintain behavior)

sterlinghirsh reviewed Sep 13, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

sterlinghirsh approved these changes Sep 13, 2022

View reviewed changes

sterlinghirsh mentioned this pull request Sep 13, 2022

Update algolia instantsearch hooks #694

Closed

sterlinghirsh previously requested changes Sep 13, 2022

View reviewed changes

Husky: pin version

943eb88

vercel bot deployed to Preview – react-commerce-prod September 14, 2022 00:45 View deployment

vercel bot deployed to Preview – react-commerce September 14, 2022 00:47 View deployment

masonmcelvain reviewed Sep 14, 2022

View reviewed changes

federicobadini reviewed Sep 15, 2022

View reviewed changes

frontend/package.json Outdated Show resolved Hide resolved

algolia hooks: update to newest version

5b76b09

vercel bot deployed to Preview – react-commerce September 15, 2022 19:47 View deployment

vercel bot deployed to Preview – react-commerce-prod September 15, 2022 19:49 View deployment

danielbeardsley added 2 commits September 15, 2022 12:53

Merge remote-tracking branch 'origin/main' into algolia-hooks--update…

2160689

…-and-patch Conflicts: pnpm-lock.yaml * Lots of conflicts, but our branch and more significant changes (and a version bump of the format). So I kept our version

Commit lock file after pnpm install post-merge

297df59

Master had added one dep since we diverged, so I kept our version of the lock file then did `pnpm install` to ensure it was up to date with with package.json requirements.

vercel bot deployed to Preview – react-commerce September 15, 2022 19:58 View deployment

vercel bot deployed to Preview – react-commerce-prod September 15, 2022 20:01 View deployment

InstantSearchProvider: drop decoding on parsed query

1f5ceab

We validated that qsModule.parse() already does url decoding and we don't need to re-do it on the return values. Also, drop some defaults on destructuring because they already happen in the code below.

vercel bot deployed to Preview – react-commerce September 21, 2022 18:25 View deployment

vercel bot deployed to Preview – react-commerce-prod September 21, 2022 18:27 View deployment

vercel bot deployed to Preview – react-commerce-prod September 21, 2022 19:56 View deployment

vercel bot deployed to Preview – react-commerce September 21, 2022 19:57 View deployment

Github Actions: Use npm to install pnpm

0020157

corepack was saying it can't find any version compatible with '7' I still don't understand why corepack helps us here, so I'm reverting to plain 'npm install' which is simpler and actually let's us install the latest version.

vercel bot deployed to Preview – react-commerce-prod September 21, 2022 20:09 View deployment

vercel bot deployed to Preview – react-commerce September 21, 2022 20:12 View deployment

fix: drop unused productlist.deviceItemType field

5155bbc

This field is unused as of 4ec07e2, so let's drop it to prevent accidental usage in the future.

vercel bot deployed to Preview – react-commerce-prod September 21, 2022 20:20 View deployment

vercel bot deployed to Preview – react-commerce September 21, 2022 20:22 View deployment

vercel bot deployed to Preview – react-commerce-prod September 21, 2022 20:37 View deployment

vercel bot deployed to Preview – react-commerce September 21, 2022 20:38 View deployment

masonmcelvain force-pushed the algolia-hooks--update-and-patch branch from aea0aae to 5155bbc Compare September 21, 2022 20:50

Github Actions: use official pnpm action to install

f00a41e

Previous one was failing for some mysterious reasons.

vercel bot deployed to Preview – react-commerce September 24, 2022 00:48 View deployment

vercel bot deployed to Preview – react-commerce-prod September 24, 2022 00:50 View deployment

masonmcelvain approved these changes Sep 25, 2022

View reviewed changes

erinemay self-assigned this Sep 26, 2022

erinemay added the QAing Under QA team review label Sep 26, 2022

erinemay removed the QAing Under QA team review label Sep 26, 2022

davidrans merged commit bc696d2 into main Sep 26, 2022

davidrans deleted the algolia-hooks--update-and-patch branch September 26, 2022 16:18

masonmcelvain mentioned this pull request Oct 5, 2022

Algola Error: RetryError: Unreachable hosts - your application id may be incorrect. If the error persists, contact support@... #765

Closed

erinemay mentioned this pull request Nov 11, 2022

Set itemType on Server #998

Merged

dhmacs mentioned this pull request Nov 28, 2022

Upgrade to React 18 #975

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Algolia hooks: update and patch #700

Algolia hooks: update and patch #700

danielbeardsley commented Sep 13, 2022 •

edited by jarstelfox

vercel bot commented Sep 13, 2022 •

edited

sterlinghirsh commented Sep 13, 2022

sterlinghirsh left a comment

danielbeardsley commented Sep 14, 2022

masonmcelvain left a comment

federicobadini left a comment

masonmcelvain commented Sep 15, 2022

federicobadini commented Sep 15, 2022

danielbeardsley commented Sep 15, 2022

danielbeardsley commented Sep 15, 2022

danielbeardsley commented Sep 21, 2022

sterlinghirsh commented Sep 21, 2022

danielbeardsley commented Sep 24, 2022

masonmcelvain left a comment

erinemay commented Sep 26, 2022

Algolia hooks: update and patch #700

Algolia hooks: update and patch #700

Conversation

danielbeardsley commented Sep 13, 2022 • edited by jarstelfox

vercel bot commented Sep 13, 2022 • edited

sterlinghirsh commented Sep 13, 2022

sterlinghirsh left a comment

Choose a reason for hiding this comment

danielbeardsley commented Sep 14, 2022

masonmcelvain left a comment

Choose a reason for hiding this comment

federicobadini left a comment

Choose a reason for hiding this comment

masonmcelvain commented Sep 15, 2022

federicobadini commented Sep 15, 2022

danielbeardsley commented Sep 15, 2022

danielbeardsley commented Sep 15, 2022

danielbeardsley commented Sep 21, 2022

sterlinghirsh commented Sep 21, 2022

danielbeardsley commented Sep 24, 2022

masonmcelvain left a comment

Choose a reason for hiding this comment

erinemay commented Sep 26, 2022

danielbeardsley commented Sep 13, 2022 •

edited by jarstelfox

vercel bot commented Sep 13, 2022 •

edited