Feature/megamenu fixes

[yunakim714]

Description

Types of Changes

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Documentation update
• Breaking change (could cause existing functionality to not work as expected)
• Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

• Removing a public function or component or prop
• Adding a required argument to a function
• Changing the data type of a function parameter or return value
• Adding a new peer dependency to package.json

Changes

• (change1)

How to Test-Drive This PR

• (step1)

Checklists

General

• Changes are covered by test cases
• CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

• There are no changes to UI

or...

• Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
• Changes comply with WCAG 2.0 guidelines levels A and AA
• Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

• Changes include a UI text update in the Retail React App (which requires translation)

[wjhsf]

(Incomplete review, but leaving comments since I'll be out.)

[alexvuong]

Nit:

Suggested change

	const [ariaBusy, setAriaBusy] = useState('true')
	const [isAriaBusy, setIsAriaBusy] = useState('true')

[kevinxh]

I might have mislead you in one of my previous comments, but it seems weird to have ariaBusy default to true, and then immediately set it to false after mount in this useEffect?

I'd imagine the busy state should based on if the fetch category promise is resolved.

[bfeister]

I don't think we need useState at all for this. We just need

       <nav
            id="list-menu"
            aria-label="main"
            aria-live="polite"
            aria-busy={root?.[itemsKey]?.filter(item => !item.loaded)?.length > 0}
            aria-atomic="true"
        >

[kevinxh]

instead of mocking isTokenValid, we could generate a valid jwt (with an expiration date 10 years from now) and paste it in here.

[wjhsf]

10 years? Dream big! What's the latest date we could possibly use without breaking things? 🤔

[kevinxh]

this code block does nothing 😛 ... it mocks the module by the actual module itself. Unless there is some black magic behind this, if so, could you add a comment on why we need this?

Suggested change

	jest.mock('./utils', () => {
	const originalModule = jest.requireActual('./utils')
	return {
	...originalModule
	}
	})

[yunakim714]

I added this because tests in this file will fail if it relies on the isTokenValid mock in the global jest-setup file but I will use the other approach to generate a valid JWT instead!

[kevinxh]

same comment for https://github.com/SalesforceCommerceCloud/pwa-kit/pull/875/files#r1067567033

[kevinxh]

Have we thought about what it might look like for the hook integration? This component seems to grown from a one liner to a pretty complicated one. I'm worried that we might end up re-write the logic during the hook integration, what can we do to mitigate that risk? (Sorry, I wish i could have said this at the beginning of the work, not when this is almost done..)

[bfeister]

The change should be all located in one spot, and if we can't call a hook inside a useEffect then I think we've uncovered a problem with the hooks library in that we still need the SDK to make calls like this. I expect we might need to continue to use the SDK in some places. If that's true, then this might be one of those spots where we don't use the hooks library

[kevinxh]

As a general best practices, a function should not mutate its input. Mutating the input leads to unexpected issues and makes code harder to understand. We could clone the input and always return that clone from the function.

    const findAndModifyFirst = (tree, childrenKey, objToFindBy, replacementObj) => {
            const clone = Object.assign({}, tree)
            // do work ...
            return clone
   }

[bfeister]

Good suggestion. @wjhsf had mentioned this and I agree that we need to not mutate the input

[bfeister]

Actually @kevinxh my preference (not a big deal) is to use object spread which handles cloning implicitly.

    const findAndModifyFirst = (tree, childrenKey, objToFindBy, replacementObj) => {
            // do work 
            return {
              ...tree
            }
   }

[wjhsf]

This feels like a bad pattern. Tests should be isolated so that they can be run independently, and a globally shared variable makes it too easy to subtly change the server between tests and not notice. To guarantee a known state, each test should use its own mock server. If you want to DRY a bit, you could move the setup/teardown hooks into setupMockServer.

(Also, you should pretty much never use global; export things instead.)

[bfeister]

There is a guaranteed known state. It is setupMockServer (and to @kevinxh 's point, we should have a global catch to prevent any get|put|patch etc. requests from touching the network).

I'm going to disagree here, our setupMockServer implementation is unnecessary boilerplate and with the introduction of the change to <CategoriesProvider /> to make an API call, we would pretty much have to add setupMockServer to every test file. Thus we are doing it globally.

We need to merge this and not rework it forever, so I'm going to vote against localizing / non-globalizing this

[wjhsf]

As long as we can guarantee a known state per test, then that's fine. Should still switch to import/export, though, unless there's weird jest nonsense going on.

[kevinxh]

I attempted to switch to import / export, but things are not working for some reasons I couldn't figure out. Let's make this a iterative change and we will need to take a closer look at those tests.

[wjhsf]

10 years? Dream big! What's the latest date we could possibly use without breaking things? 🤔

[wjhsf]

Sometimes I like to split things up to avoid extra nesting.

        const promises = root?.[itemsKey]?.map(async (cat) => {
          // ...
        })
        await Promise.all(promises)

[wjhsf]

Why is this error returned?

[wjhsf]

This is still an outstanding issue. This will result in the category items being Category | Error, which does not seem like what we want.

[wjhsf]

(This hasn't been marked resolved, but I just want to highlight that this is still an important issue to address.)

[bfeister]

Yikes, so much controversy on this one, most of it caused by me forgetting to remove that tree walking code that wasn't needed.

Great work pushing through 🏅

[wjhsf]

This is still an outstanding issue. This will result in the category items being Category | Error, which does not seem like what we want.

[wjhsf]

Should have an explicit condition here for if there's nothing in storage.

[wjhsf]

This will log the error to the browser console without any context, and then do nothing. Is that what we want? Do we have a fallback / error state that the menus can show?

[yunakim714]

#875 (comment) - Regarding this issue and the one referenced, would it be better to throw err ?

[wjhsf]

They're different scenarios. For the other comment, I don't think we want error handling at that layer (don't catch/rethrow), just let it bubble up to whatever error handling we do have.

I'm not sure what the proper solution is, here. Depends on what happens when the category data requests fail. Probably not log and ignore, which is the current state. Maybe just let the error bubble up to react and then have proper error handling in a follow up ticket.