PLANET-5017: Re-implement Spreadsheet block frontend in React

[pablocubico]

Porting the Spreadsheet block's frontend to React.

Ref: https://jira.greenpeace.org/browse/PLANET-5017

[pablocubico]

A few notes about this:

0. I need to squash some commits!

1. I tried to avoid the render_callback part on the PHP side by just rendering the node in which we inject the React component in the save() method of the editor, but using the save() method in the Block definition triggers a validation error in the Editor, as our current blocks return null, any value different than null will trigger a validation error and ask you to "Resolve" the block (yuck!). We could just return an HTML node there to render the React content in the frontend, but changing the save() function will make the current spreadsheets unusable. I just made it a "template-less" render, it just renders a node. Ideally I would love to skip the PHP side in these cases, but let's stick to null and render_callback for now. ( This is a long standing controversy in Gutenberg, there are a few discussions about it out there:
   Block Validation - Replace with expected WordPress/gutenberg#10444 )

2. I added an endpoint to transform the CSV to JSON and keep using WP's cache store the Spreadsheet as JSON data. I was thinking it would be fast since the cache is persistent, so the data should be obtained from cache even from different visitors, but then @Inwerpsel came in and said: "Hey, why don't you just get the spreadsheet from the client side?", which is pretty much the same from the component's point of view, is just a bunch of JSON rows. So, as Pieter said, that could avoid WP's overhead, but I guess it could add some overhead to the Javascript side since we need to turn that CSV into something iterable, which is super simple but if the Spreadsheet is long, it may have a performance impact on low-end cellphones. Which is also a concern for React, the React embed is not that big, but it adds quite a bunch of logic for the browser to process, despite the KB size. There is also some cost in the DNS Lookup and connecting to Google if we load it directly from the client side.

So, I'm not sure about (2), maybe we can test these assumptions? Is the WP overhead that bad? Is parsing the CSV lines to JSON super fast? What do you think?

Ping @Inwerpsel @sagarsdeshmukh @comzeradd

[comzeradd]

Regarding point 2: I'm leaning mostly towards 1st option. Since we already need to render the block in the backend, it seems less complicated and faster for the end-user to use WP cache store. Unless I'm missing something, it also feels more work to do the 2nd option. Which is not necessarily bad if that's the proper way of implementation. But maybe this block is a good opportunity to see how this works and do something similar with WYSIWYG blocks. Rendering on the client-side could be a very good option if we didn't render the block in the backend at all (eg. the native Embed block for Facebook).

[Inwerpsel]

Regarding 1. Seems like the most sensible thing to do indeed. Maybe we can set this up in a generic way for all blocks that are rendered frontend? We could use

<div
  data-block="{{ block.id }}"
  data-attributes="{{ block.attributes.toJson }}"
></div>

for all blocks.

Actually, maybe then we can also circumvent the save() issue if we would persist this markup in JS 🤔 Because we would have only one "block shape change" to deal with, from null to this format, which we can write a resolver for, probably. We would only need to write one as the element and attributes don't need to change to support other blocks. Even if the html would be changed externally, we can write a resolver for that as the content can be entirely reconstructed from the block attributes in the html comment.

Maybe worth a shot? Then we wouldn't need the render on the backend, which is basically just turning one representation of the block's data into another one (block comment -> data attributes).

As for 2. I think the CSV parsing should be really fast and only an issue for huge spreadsheets, like 100k or more rows, far beyond what would make sense to include on a page. But we can try if we want to be sure. Possibly we can get this data as JSON too from the Google sheets API.

The reason I remarked it was because I assume that Google sheets has a pretty extensive CDN to deliver this data efficiently anywhere in the world, so very likely the round trip to our server will be significantly longer, even without WP overhead. Note that currently, even though a placeholder for the blocks is rendered on the backend, the data of the sheet doesn't come with that. The frontend needs to do a separate call to a custom endpoint before it can start rendering. So for the end user the difference in speed is between how fast our server can serve that endpoint vs how fast Google sheets CDN can serve a static resource.

Though serving it from our backend also has some advantages, as it ensures the sheet will work regardless of possible issues that might prevent the browser from getting the data from Google. There might be some other considerations when including data from external domains.

I'm ok with the current approach of caching it on our server as there's probably no noticeable difference to the end user in most cases. Probably the additional load on our servers is also negligible.

[pablocubico]

Hey @Inwerpsel , sorry for the delay.

Regarding 1:
I tried doing that in the save() method but my version was using data-url, a "per attribute" property, which does not scale well if we change the attributes..., it actually makes sense if to only pass "data-attributes" to it and extract the ones we want in the frontend renderer. I'll give it a try and use the "deprecations" feature of Gutenberg to bypass the validation issue.

Regarding 2:
Alright, let's keep the current version which uses the cache and could help in avoiding some DNS latency or Google issues... in case Google has any. We can always change it to a client fetch since it's pretty much the same from the frontend side.

[pablocubico]

@comzeradd @Inwerpsel

Branch updated to latest master. I removed the render_callback from the PHP side and created a generic method to render blocks in the frontend. The save() provides a className which includes the block's name but also any additional classes from the block styles feature, so it's quite handy.

I wrote a deprecation handler to avoid any warnings from the save() method change. Looks pretty good. I also ported the CSS variables from the spreadsheet colors feature from Twig to React.

[Inwerpsel]

Great work 💯 Tested locally and it's working, just a couple of things in there that we need to fix (mainly the block not updating when you change the url in the editor), and couple of small tweaks/extractions. But it's almost there 👍

[pablocubico]

Hey @Inwerpsel @comzeradd , branch rebased to master, this is currently deployed in the Mars environment.

Addressed all the comments in the review, applied most of the suggestions. I didn't decouple the rendering from the API fetch, I think we can do that later if the requirements change, for example, if we are planning to fetch spreadsheets from other sources than Google docs.

I also considered registering a Redux Store in the Gutenberg side to handle the data, using the withSelect/withDispatch/subscribe combo there instead of apiFetch, that way we could use the Redux debugger to monitor the requests, even do rewind, play, and such, but it has no real benefit from the frontend side, as there are no Stores registered there, and seems overkill. For the moment, WP is still focused on the Editor side for their React stuff.

[Inwerpsel]

Looks great 💯 I have some final nit picking comments, nothing blocking so can approve already, but would be nice if you could have a look. As for decoupling, I guess it would still make sense to do it already. It would have some other benefits, mainly it would make the SpreadsheetFrontend component less complex. But we can indeed take that up later 👍