🦋 Changeset detected

Latest commit: 9ecee4e

The changes in this PR will be included in the next version bump.

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

📦 Next.js Bundle Analysis for @faustwp/getting-started-example

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

This is working well for me in Chrome. Noted internally that it does not work in Safari, but seems to be related to existing auth/cookie issue in Safari, and not a blocker for merge here.

Would be nice to have some tests to confirm the Edit node appears as expected to avoid regressions.

For the tests that are failing, this seems due to our introduction of useRouter, we'll have to mock it. @TeresaGobble you could do something like this:

Do a wildcard import of next/router:

import * as nextRouter from 'next/router';

Then in the Toolbar tests that are failing, mock the useRouter call:

  const useRouterSpy = jest.spyOn(nextRouter, 'useRouter').mockReturnValue({
    query: {},
  } as any as nextRouter.NextRouter);

For example:

test('renders the toolbar if user preference is true', async () => {
  expect.assertions(1);
  mockIsAuthenticated = true;

  const mockUseQuery = {
    data: {
      viewer: {
        shouldShowFaustToolbar: true,
      },
    },
  } as any as QueryResult<unknown, unknown>;

  const useQuerySpy = jest
    .spyOn(apollo, 'useQuery')
    .mockReturnValue(mockUseQuery);

  const useRouterSpy = jest.spyOn(nextRouter, 'useRouter').mockReturnValue({
    query: {},
  } as any as nextRouter.NextRouter);

  const dom = render(<Toolbar />);

  const navElement = await waitFor(() =>
    queryByAttribute('id', dom.container, 'wpadminbar'),
  );

  expect(navElement).toBeInTheDocument();
});

+1 to @mindctrl's point about adding another test to avoid regressions. We could mock certain values from the useRouter to do that:

  const useRouterSpy = jest.spyOn(nextRouter, 'useRouter').mockReturnValue({
    query: {
      p: '123',
      typeName: 'Post'
    },
  } as any as nextRouter.NextRouter);

This is working well for me in Chrome. Noted internally that it does not work in Safari, but seems to be related to existing auth/cookie issue in Safari, and not a blocker for merge here.

Would be nice to have some tests to confirm the Edit node appears as expected to avoid regressions.

Thank you John! Still working on getting the tests to pass here for the packages on Node.js 16 and 18- the logic has changed a bit with this implementation. 👍

Hi @blakewilson and @mindctrl ! Thank you so much for reviewing this for me!

My test packages for Node.js 16 and 18 failed upon adding our regression tests for the Edit Post Node in previews. I've confirmed that they're passing locally, but in our GitHub actions the test is receiving one assertion call instead of two, and expects 3 items but receives 4. I've dug around but can't seem to find an explanation for the discrepancy. Maybe something is unexpected about the way GitHub is handling expect.assertions(number) in this scenario? I've also rerun the failed checks, no change.

Hi @blakewilson and @mindctrl ! Thank you so much for reviewing this for me!

My test packages for Node.js 16 and 18 failed upon adding our regression tests for the Edit Post Node in previews. I've confirmed that they're passing locally, but in our GitHub actions the test is receiving one assertion call instead of two, and expects 3 items but receives 4. I've dug around but can't seem to find an explanation for the discrepancy. Maybe something is unexpected about the way GitHub is handling expect.assertions(number) in this scenario? I've also rerun the failed checks, no change.

It might be that the assertions fail because of the missing FAUST_SECRET_KEY:

https://github.com/wpengine/faustjs/actions/runs/6041182270/job/16393924705?pr=1556#step:6:513