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

#93 React Init WordPress Block Changes #153

Merged
merged 4 commits into from
Jan 5, 2024

Conversation

bd-viget
Copy link
Contributor

@bd-viget bd-viget commented Jan 5, 2024

Summary

This PR makes some adjustments to the Typescript files and structure to use wp-scripts to build the JS asset files for the custom Bidding Block.

@nick-telsan I wasn't 100% sure the purpose of this block (is it supposed to replace the Bid Now block, or accompany it, or does this render other stuff, or is this the Node service element... I have no idea). I left the final output relatively similar, but removed the placeholder attributes for now with the assumption we will retrieve these another way.

Issues

Notes

  • First and foremost, this is using Webpack, which I know is not something you were a fan of. My hope is that you can use what I have as a starting point and maybe modify things to work the same way, except with the tools you prefer, but unfortunately that's beyond my knowledge of build tools.
  • Even though everything compiles just fine, there are some red underlines in the edit.tsx, index.tsx, and save.tsx files and I'm not really sure how to resolve them (if possible).
    • Most have TS2307: Cannot find module @wordpress/[module-name] or its corresponding type declarations.
    • There's one on the import of block.json: TS2732: Cannot find module ./block.json. Consider using --resolveJsonModule to import module with .json extension. - I don't know what that means, but everything is working just fine.
    • There's a yellow underline in save.tsx: Void function return value is used - It still compiles correctly, so not sure if it's a big deal.
  • Typically when I build a custom block, I will use a server-side rendering method, especially when the block is dynamic. I don't know if this is React or WordPress or what, but the Edit property HTML/Markup is supposed to match the Save property, and if they don't match up, what you end up with is an error in the block that says the block contains invalid markup. To avoid this, I think the trick is to make as many elements as dynamic as possible. (You may not run into this issue, but if you do, it may require making some adjustments or using another component).
  • Because of the item above, using the <div id= data-xx= element type initializer approach isn't really going to work. The JS relies on the element to be present in order to retrieve the Auction ID, and that just ends up not working on the front-end or causes the markup to be different. To workaround this, I gave the block an attribute of auctionId, and am loading it using the WordPress useSelect module to grab the Current Post ID. From there, the rest of the data can be populated dynamically, however, I wish it was as simple as requesting some meta using the Post ID. Some of the values may be obtainable that way, but most of them are calculated with our goodbids()->auctions... API, so what we will probably need to do is go ahead and start setting up some custom WordPress REST API endpoints to retrieve this information (or maybe we can do it in 1 endpoint). /cc @clatwell
  • A lot of the formatting changes are from npm run format:fix, so sorry if it adds a lot of noise/changes, but I couldn't even commit these files without those updates.
  • If we need to also load some CSS, we can include a reference to it in the block.json and add a file inside the blocks/block-name folder.
  • Because the dist folder gets emptied (due to emptyOutDir: true), everything goes into the build directory. This is probably not ideal, however, I ran into some blockers here and wanted to get your feedback before doing anything else.
    • I'm not sure if it would be OK to disable emptyOutDir so all the files can go in the same folder, but that may be all we need to do.
    • Since wp-scripts uses webpack, we're not using vite to compile the custom React block, but if you know of a way to make this work, by all means feel free to change it.
    • I configured in both webpack.config.cjs and tsconfig.json to use "outDir": "./blocks/react", however, that doesn't appear to be working (the files are in build/blocks). If you know how to configure this properly, it seemed to make sense to include these blocks with the rest of the blocks, however, if it just isn't right at all, I guess we just remove those lines?
    • I may have added a few lines to the tsconfig JSON files, I was just following the instructions I found to get Typescript working with wp-scripts, but if you look at it and know what's going on and think we don't need them, I trust your judgment on removing/changing those values.
  • Running npm run build will compile everything. npm run start will put wp-scripts in dev mode, however frequent changes to the block could result in a "block contains invalid markup" type error (mentioned before), and may require you to either remove the block and re-add it, or run npm run build. Again, not my area of expertise.
  • I'm calling registerBlockType() in src/blocks/bidding/index.tsx, however, I'm also calling register_block_type() in src/classes/Blocks/Blocks.php. I don't think the PHP one is doing much of anything, but the allowed_block_types_filter appears to be the only way to restrict a block to a particular post type (although I'm doing it wrong anyway). You may find a few things like this in here, but once I got it working I was too afraid to change stuff, so figured we could clean it up later.
  • I added wp-prettier although I didn't implement it, however, I was curious if we should be using it since we're in a WordPress plugin? I didn't look too much into this other than discovering it exists.
  • The block name, folder, title, and icon are totally fine to change to whatever. Just be sure to update it in the Block.php class. (I think I might consider loading the blocks array via the config.json file which I haven't really been utilizing much, but need to.

I think that's about all the notes I can detail out for this. If something isn't working or making sense, hit me up and we can go over it! Thanks for your patience!!

@bd-viget bd-viget requested a review from nick-telsan January 5, 2024 05:34
@bd-viget bd-viget self-assigned this Jan 5, 2024
@nick-telsan
Copy link
Contributor

The Block Itself

This will ultimately replace the bid-now block. It will be required on all auction pages.

Webpack

I will defer to you on what we should use for minimal WP friction. Originally we discussed Vite as it is our default for JS projects.

If we're using Webpack, we should move the Tailwind config into Webpack as well. No need to have 2 bundlers.

Red Underlines

This is just installing the appropriate @wordpress/[packages] in the package.json so React knows where to look.

It does look like these packages are either not able to properly infer types for TypeScript. Installing the appropriate packages resolves the errors you were seeing, but creates a couple of new ones like Property 'getCurrentPostId' does not exist on type 'never'. We can work around it. I have no idea what that issue is.

Dynamic Block / Server Side Rendering

I think I understand what you're saying. Essentially, all of the React app is conditionally rendering things based on user state and auction state. This can't be saved since it will change based on changing user and auction state, so we have to make everything dynamic.

Dist folder

We can change that. All that is doing is making sure we have a clean directory, but that is only affecting local dev.

Swapping everything over to Webpack would also work.

CSS

We shouldn't need unique CSS for this block (ideally), just the Tailwind config that Nathan's added with Vite.

Formatting and Linting

We can move all of that into the WP scripts versions.

@@ -0,0 +1,30 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

@bd-viget How hard of a requirement is it for save.tsx and edit.tsx to match? For example, while we are in the editor, we don't really want or need make a websocket connection, but we definitely need that on the page visible to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think as long as you load additional components conditionally (such as where the <ul> is getting included) and they load other React Components, I don't think Gutenberg makes a fuss, but it's been a while since I've worked with custom blocks, so I hope you don't end up in a trial/error situation. If that happens and the block keeps breaking, maybe get to a point where you like and I can try to see if I can make Gutenberg happy :D.

Also, just to clarify, the files themselves don't need to match, just the returned markup can't be like:
save.tsx

<div>This is my block.</div>

edit.tsx

<p><span>This is something completely different.</span></p>

(this may not be the best of examples, sorry!)

@nick-telsan nick-telsan requested a review from cwmanning January 5, 2024 15:03
@bd-viget
Copy link
Contributor Author

bd-viget commented Jan 5, 2024

@nick-telsan

Webpack

I'll be honest, I don't quite understand the difference in Webpack and Vite as far as how 1 functions different from the other. Is it like a preference thing, like choosing gulp over grunt, or something else? I know wp-scripts is using it, but maybe wp-scripts can be replicated to do the same thing with Vite? Ultimately, the end goal is to have the same output generated by Vite that is generated by Webpack, and if that's the case, I really don't care, and I don't think there are any rules about using Vite. There may also be someone else that has already done this and has a Github Repo or Blog post on the subject (I just haven't been trained to do that sort of Google-fu)

If we're using Webpack, we should move the Tailwind config into Webpack as well. No need to have 2 bundlers.

Works for me! Whatever makes sense as far as consolidation to streamline this I'm totally good with.

This is just installing the appropriate @wordpress/[packages] in the package.json so React knows where to look.

Ok, so does this mean we need to put those in package.json, or something to do with PhpStorm? - NVM, looks like you answered this next.

It does look like these packages are either not able to properly infer types for TypeScript. Installing the appropriate packages resolves the errors you were seeing, but creates a couple of new ones like Property 'getCurrentPostId' does not exist on type 'never'. We can work around it. I have no idea what that issue is.

Yeah I was already seeing that yellow underline there, but again, it compiles so not terribly worried about that.

I think I understand what you're saying. Essentially, all of the React app is conditionally rendering things based on user state and auction state. This can't be saved since it will change based on changing user and auction state, so we have to make everything dynamic.

Exactly, and if you want to use one of the core blocks as an guide/example, you can find them here. (Maybe check out the Comments block?)

We can change that. All that is doing is making sure we have a clean directory, but that is only affecting local dev.

👍

Swapping everything over to Webpack would also work.

👍 If you know how to do that, I'm all for it!

We shouldn't need unique CSS for this block (ideally), just the Tailwind config that Nathan's added with Vite.

👍

We can move all of that into the WP scripts versions.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nick-telsan Crap! Sorry, this file was not supposed to be committed!

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed it in #154!

@nick-telsan
Copy link
Contributor

@bd-viget

I'll be honest, I don't quite understand the difference in Webpack and Vite as far as how 1 functions different from the other.

Webpack tends to just be harder to work with than Vite. Most of our Ruby projects or standalone JS projects benefit from Vite over Webpack. But, in this case, Vite has been harder to finesse into WordPress. I think we're good to just run with wp-scripts and webpack approach since those work well with WordPress.


For those outstanding issues, what I want to do is merge these PRs into #115 and just clean up our PRs a bit.

We can leave Vite, formatting, and linting as they are for a minute while we get everything stable, and then knock those out.

I'm open to either
a) not merging in #115 until we get the wp-scripts, Vite, and Tailwind stuff cleaned up

or

b) merging in #115 with Vite serving Tailwind and then adjusting that in another PR.

- Adds packages for WP Script blocks
- Adjust TS Config
@nick-telsan nick-telsan merged commit 5c09b82 into nt/93-react-init Jan 5, 2024
1 check passed
@nick-telsan nick-telsan deleted the bd/93-react-init branch January 5, 2024 19:23
nick-telsan added a commit that referenced this pull request Jan 9, 2024
* [#93] React Init

- Adds prettier
- Adds lint-staged and husky
- Adds bin scripts
- Adds initial react app

* [#93] React Init WordPress Block Changes (#153)

- Custom React Block for Bidding
- Adds packages for WP Script blocks
- Adjust TS Config

* [#94] Metrics but WP (#155)

- Adds driver and metrics component
- Adjusts Tailwind content paths
- Adds inputs for new components
- Adds script to handle components in dev mode
- Moves metrics into the WP block

* [#95] Countdown Timer (#148)

- Adds countdown timer component and functions
- Adjusts initial state for demo
- Adjusts font styles
- Fixes gitignore build

* [#96] Bid Button (#149)

- Adds clsx
- Adds bid button

* [#97] Free Bids (#150)

- Adds free bids button
- Adds demo data

* [#100] Participation (#151)

- Adds participation component

* [#101] Free Bids (#152)

- Adds free bids promo
- Adds temporary hand icon

* [#140] Websocket Connection (#167)

- Installs zod and zustand
- Adds attributes to block.json
- Adds auction store
- Adds types
- Changes initial state to data attributes
- Adjusts use of data attributes and useAuction
- Adds websocket connection
- Hardcodes WS URL for now.
nick-telsan added a commit that referenced this pull request Jan 9, 2024
* [#93] React Init

- Adds prettier
- Adds lint-staged and husky
- Adds bin scripts
- Adds initial react app

* [#93] React Init WordPress Block Changes (#153)

- Custom React Block for Bidding
- Adds packages for WP Script blocks
- Adjust TS Config

* [#94] Metrics but WP (#155)

- Adds driver and metrics component
- Adjusts Tailwind content paths
- Adds inputs for new components
- Adds script to handle components in dev mode
- Moves metrics into the WP block

* [#95] Countdown Timer (#148)

- Adds countdown timer component and functions
- Adjusts initial state for demo
- Adjusts font styles
- Fixes gitignore build

* [#96] Bid Button (#149)

- Adds clsx
- Adds bid button

* [#97] Free Bids (#150)

- Adds free bids button
- Adds demo data

* [#100] Participation (#151)

- Adds participation component

* [#101] Free Bids (#152)

- Adds free bids promo
- Adds temporary hand icon

* [#140] Websocket Connection (#167)

- Installs zod and zustand
- Adds attributes to block.json
- Adds auction store
- Adds types
- Changes initial state to data attributes
- Adjusts use of data attributes and useAuction
- Adds websocket connection
- Hardcodes WS URL for now.
nick-telsan added a commit that referenced this pull request Jan 9, 2024
* [#93] React Init

- Adds prettier
- Adds lint-staged and husky
- Adds bin scripts
- Adds initial react app

* [#93] React Init WordPress Block Changes (#153)

- Custom React Block for Bidding
- Adds packages for WP Script blocks
- Adjust TS Config

* [#94] Metrics but WP (#155)

- Adds driver and metrics component
- Adjusts Tailwind content paths
- Adds inputs for new components
- Adds script to handle components in dev mode
- Moves metrics into the WP block

* [#95] Countdown Timer (#148)

- Adds countdown timer component and functions
- Adjusts initial state for demo
- Adjusts font styles
- Fixes gitignore build

* [#96] Bid Button (#149)

- Adds clsx
- Adds bid button

* [#97] Free Bids (#150)

- Adds free bids button
- Adds demo data

* [#100] Participation (#151)

- Adds participation component

* [#101] Free Bids (#152)

- Adds free bids promo
- Adds temporary hand icon

* [#140] Websocket Connection (#167)

- Installs zod and zustand
- Adds attributes to block.json
- Adds auction store
- Adds types
- Changes initial state to data attributes
- Adjusts use of data attributes and useAuction
- Adds websocket connection
- Hardcodes WS URL for now.
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