Bug: Fixing null/undefined theme argument for the WordPressBlocksProvider

[colinmurphy]

Tasks

• I have signed a Contributor License Agreement (CLA) with WP Engine.
• If a code change, I have written testing instructions that the whole team & outside contributors can understand.
• I have written and included a comprehensive changeset to properly document the changes I've made.

Description

Fixed an issue with the theme argument for the WordPressBlockProvider to allow for null or undefined #1986

The condition to check if WordPressThemeContext is not needed as theme is an optional argument as per our docs - https://faustjs.org/reference/wordpressblocksprovider

e.g.

      <WordPressBlocksProvider
        config={{
          blocks,
          theme: null
        }}>
        <Component {...pageProps} key={router.asPath} />
      </WordPressBlocksProvider>

So setting the theme argument null, undefined or not at all should not result in an error.

How to replicate the issue

If you setup an Next.js app with Faust Blocks you should be able to replicate the error.

cd directory/
npx create-next-app@latest
cd my-app #assuming you set my-app as the project name
npm install @faustwp/blocks

src/pages/_app.js

import "@/styles/globals.css";
import { WordPressBlocksProvider } from "@faustwp/blocks";
import blocks from "../wp-blocks";


export default function App({ Component, pageProps }) {

  return (
    <WordPressBlocksProvider config={{ blocks, theme: undefined}}>
      <Component {...pageProps} />
    </WordPressBlocksProvider>
  );
}

src/pages/index.js

import { WordPressBlocksViewer } from "@faustwp/blocks";

export default function Home() {
  return (
    <div>
      <h1>Home</h1>
      <WordPressBlocksViewer blocks={
[
  {
    "name": "core/paragraph",
    "__typename": "CoreParagraph",
    "apiVersion": 3,
    "isDynamic": false,
    "attributes": {
      "cssClassName": null,
      "backgroundColor": null,
      "content": "A JavaScript framework that makes building headless WordPress simple and easy.",
      "style": null,
      "textColor": null,
      "fontSize": null,
      "fontFamily": null,
      "direction": null,
      "dropCap": false,
      "gradient": null,
      "align": null
    }
  },
  {
    "name": "core/paragraph",
    "__typename": "CoreParagraph",
    "apiVersion": 3,
    "isDynamic": false,
    "attributes": {
      "cssClassName": null,
      "backgroundColor": null,
      "content": "A familiar publisher experience, providing a cohesive experience with wp-admin. Easily extensible with built in filters, empowering developers to customize for different use cases.",
      "style": null,
      "textColor": null,
      "fontSize": null,
      "fontFamily": null,
      "direction": null,
      "dropCap": false,
      "gradient": null,
      "align": null
    }
  }
        ]
      } />
    </div>
  );
};

src/wp-blocks/index.js

import { CoreBlocks } from "@faustwp/blocks";

export default {
	...CoreBlocks,
};

if you run npm run dev you will get the following error:

useBlocksTheme hook was called outside of context, make sure your app is wrapped with WordPressBlocksProvider

The following PR fixes that issue.

Further Discussion

After debugging this issue with @moonmeister we also found that the theme argument appears to be redundant.
If you have a look at any of the blocks, it is only passed to the getStyles component which doesn't use the theme argument.

e.g.

https://github.com/wpengine/faustjs/blob/canary/packages/blocks/src/blocks/CoreParagraph.tsx
https://github.com/wpengine/faustjs/blob/canary/packages/blocks/src/utils/get-styles/getStyles.ts
https://github.com/wpengine/faustjs/blob/canary/packages/blocks/src/utils/get-styles/getTextStyles.ts#L10

There is a question on whether this is needed as none of the style components use this argument.

Related Issue(s):

#1986

Testing

As described above using a next.js app

Screenshots

Documentation Changes

Dependant PRs

[changeset-bot]

⚠️ No Changeset found

Latest commit: c53812b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

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

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

This PR introduced no changes to the JavaScript bundle! 🙌

[colinmurphy]

@moonmeister Can you review this please and see if you can still replicate issue #1986 please

[colinmurphy]

@moonmeister I need to update this PR as some of the logic isn't correct

[colinmurphy]

@moonmeister This is ready for review.

[moonmeister]

Can you explain why/how this fixes the issue?

[colinmurphy]

@moonmeister

What PR should solve is the issue around not allowing null for the theme argument for the WordPressBlockProvider. FWIW I couldn't ever replicate the bug but this is what the PR should do

1. Checks if both WordPressBlocksContext and WordPressThemeContext is defined, if not then it should throw an error (spotted a typo in the code)

2. Then it should allow WordPressThemeContext values to be null. It will only throw an error if undefined

[colinmurphy]

@moonmeister

Thank you for help on debugging this issue. I have updated the PR. I have also updated details on how to replicate the issue and also how it appears theme might be a redundant argument.

[theodesp]

I think we should revert useBlocksTheme to its original implementation because it contains a critical check to ensure that an error is triggered when the hook is used outside of the WordPressBlocksProvider.

import React from 'react';
import { useBlocksTheme } from '@faustwp/blocks';

function MyComponent() {
  const theme = useBlocksTheme();

  return <div>Current Theme: {JSON.stringify(theme)}</div>;
}

export default function App() {
  return (
    <div>
      {/* Forgot to wrap the app with WordPressBlocksProvider */}
      <MyComponent />
    </div>
  );
}

I think the code needs to change is in WordPressBlocksProvider so that we conditionally wrap the content with WordPressThemeContext if the theme exists. Something like that:

export function WordPressBlocksProvider(props: {
  children: React.ReactNode;
  config: WordPressBlocksProviderConfig;
}) {
  const { children, config } = props;
  const { blocks, theme } = config;

  const content = (
    <WordPressBlocksContext.Provider value={blocks}>
      {children}
    </WordPressBlocksContext.Provider>
  );

  return theme ? (
    <WordPressThemeContext.Provider value={theme}>
      {content}
    </WordPressThemeContext.Provider>
  ) : (
    content
  );
}

[moonmeister]

This will still throw an error cause we call useBlocksTheme in every core block even though, as best I can tell, we don't ever leverage that theme. So while this could be a good fix, it would be incomplete, I believe.

[theodesp]

We could just remove the useBlocksTheme usages inside the blocks. Or maybe even better, always assign a default value for theme as {} so we won't have to change anything else.

export function WordPressBlocksProvider(props: {
  children: React.ReactNode;
  config: WordPressBlocksProviderConfig;
}) {
  const { children, config } = props;
  const { blocks, theme = {} } = config;

  return (
    <WordPressThemeContext.Provider value={theme}>
      <WordPressBlocksContext.Provider value={blocks}>
        {children}
      </WordPressBlocksContext.Provider>
    </WordPressThemeContext.Provider>
  );
}

[colinmurphy]

@theodesp I might be wrong here but would we also need to then update the getStyles function and style blocks (e.g. getBorderStyles as useBlocks is used as the theme argument in all core blocks for this function.

[colinmurphy]

We could just remove the useBlocksTheme usages inside the blocks. Or maybe even better, always assign a default value for theme as {} so we won't have to change anything else.

export function WordPressBlocksProvider(props: {
  children: React.ReactNode;
  config: WordPressBlocksProviderConfig;
}) {
  const { children, config } = props;
  const { blocks, theme = {} } = config;

  return (
    <WordPressThemeContext.Provider value={theme}>
      <WordPressBlocksContext.Provider value={blocks}>
        {children}
      </WordPressBlocksContext.Provider>
    </WordPressThemeContext.Provider>
  );
}

Thanks @theodesp this does work setting a default for the theme so I am just going to update the PR shortly.