Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React components #143

Merged
merged 45 commits into from
Dec 6, 2022
Merged

React components #143

merged 45 commits into from
Dec 6, 2022

Conversation

psrpinto
Copy link
Member

@psrpinto psrpinto commented Dec 5, 2022

I apologise for the large PR. It was an iterative process, so the history isn't super clean. If you want to review the code, it's probably best to do so commit-by-commit, looking at the diff as whole is probably overwhelming.

There are only minimal functional changes (all pluses IMHO), and I've tested a bunch. Will do more extensive testing before issuing the next release. The majority of the changes is "just" moving things around, refactoring in order to remove duplication and for clarity.


Context

Currently, the project is structured around two "targets" (block and popup) with their own separate build. The result of the build is two completely separate targets:

  • build/frontend/block
  • build/frontend/popup

In addition, for each target, two separate bundles are produced: the iframe and the parent. The iframe bundle contains assets loaded inside the iframe, and the parent contains assets required for bootstrapping the iframe.

This is achieved by having two separate vite "projects" (one for block, one for popup), each producing two bundles, one for iframe, one for parent. The result of this is that there are four different bundles (block-iframe, block-parent, popup-iframe, popup-parent) that the WordPress side of things needs to deal with.

Additionally, in WordPress land, the block has two separate usages: one in the editor completely in JavaScript, and another in the front when the block gets rendered, in PHP.

The problem

Currently the iframe is considered to be the center of the project, with the parent being somewhat of an after-thought. This is visible in the fact that the main vite.config.ts is the one of the iframe, with the parent having a vite-parent.config.ts.

However, I think that the fact that we use an iframe to render Chatrix onto a page is a detail of the current implementation. We could imagine a future where that would no longer be the case. Having the project structured around an implementation detail is IMHO the root of the problem, and results in several issues:

  • Making changes is cumbersome since they need to be made, potentially, in seven different places: 2x development environment, 2x WordPress iframe, 2x WordPress parent, 1x Block editor.
  • Need to resort to scoping yarn commands, e.g. yarn start:popup.
  • Not possible to have both the block and popup running simultaneously in development environments.
  • Total bundle size is doubled since iframe assets are present in both targets, despite being exactly the same.
  • Directory structure is confusing, with unclarity in what belongs in the iframe and what's in the parent.
  • Having two completely separate targets also means there are two service workers, one for block, one for popup. For the user, this means they will need to login on each target, and apply updates on each target as well.

The solution

The solution implemented in this PR is to effectively make the "root" of the project the "parent" (but just call it app), with the vite config in vite.config.ts. The iframe then becomes the "secondary" target, with its own vite-iframe.config.ts.

This means that you can now just do yarn start and have both the block and the popup onscreen simultaneously. No longer need to run yarn start:block, then ctrl+c and yarn start:popup.

In addition, in this PR, we introduce two React components (<Block> and <Popup>) that allow you to render Chatrix onto the page, and abstract the fact that an iframe is used internally. Two functions (renderBlock() and renderPopup()) were also added to make it easy to render the components from wherever, without needing to deal with React.

This strategy reduces bundle size in half, since there is no longer duplication of iframe assets. There's also now a single service worker for both block and popup.

Summary of changes

(Not in commit order, not extensive, just a summary.)

  • Add <Chat> component. Abstracts rendering the iframe.
  • Add <Block> component. Uses <Chat> internally.
  • Add <Popup> component. Uses <Chat> internally.
  • Add renderBlock() and renderPopup() functions to abstract rendering the <Block> and <Popup> components, respectively. These are the only functions the WordPress side of things needs to know. There's an exception to this rule for the editor, there the <Block> component is used directly by the Edit component.
  • Refactor script registration and enqueuing in WordPress now that there's a single bundle. Make sure scripts are only enqueued when actually needed.
  • Have a single vite "project" for both the block and the popup targets.
  • Refactor directory structure so that all frontend code is in frontend/, and all iframe code is in frontend/iframe.
  • In development environment, display both the block and the popup on the same page.

Screen captures

Disk usage

disk-usage

Block and popup on same page in development

dev-env

Directory structure

Before After
directory-structure-before directory-structure-after

@psrpinto psrpinto self-assigned this Dec 5, 2022
@psrpinto psrpinto marked this pull request as ready for review December 5, 2022 18:58
@psrpinto psrpinto added this to the 0.4 milestone Dec 5, 2022
Copy link
Member

@ashfame ashfame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried running it locally, works nicely. Thanks for doing this :)

@ashfame
Copy link
Member

ashfame commented Dec 6, 2022

Is this file still relevant? Perhaps require an update with this PR?

I found it wanting to add custom css for Chatrix.

@psrpinto
Copy link
Member Author

psrpinto commented Dec 6, 2022

Good point @ashfame, I think this file can be removed as the styles are defined on the <Block> component now. I'm working on a follow-up PR that simplifies how the block (frontend/block/) gets rendered, so I'll address this there.

@psrpinto psrpinto merged commit 2bd3a21 into main Dec 6, 2022
@psrpinto psrpinto deleted the components branch December 6, 2022 14:52
@psrpinto psrpinto mentioned this pull request Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants