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

Major structural overhaul to the repo #195

Merged
merged 39 commits into from
May 11, 2022
Merged

Conversation

ryanwelcher
Copy link
Contributor

@ryanwelcher ryanwelcher commented Mar 11, 2022

This PR is an attempt to simplify the structure of the repo by using a single build process for all of the examples.

Benefits:

  • Allows the repo to show off the power and flexibility of the @wordpress/scripts package by leveraging both the built-in handling of multiple blocks while also adding our own functionality via an extended webpack.config.js file.
  • Simplifies the repo by no longer needing to leverage Lerna to build out the various examples.
  • Allows all files to be stored in a single build directory. This will make using an Action to generate a new .zip archive much easier and potentially allow us to not have to version the build directory - which is not a common practice in the wild.

Changes:

  • New blocks must be added to the src directory as would be done normally when using the scripts package.
  • Any non-block examples such as data API examples or slot fill examples must be added to the non-block-examples directory for the build process to find them

@gziolo
Copy link
Member

gziolo commented Mar 16, 2022

Allows all files to be stored in a single build directory. This will make using an Action to generate a new .zip archive much easier and potentially allow us to not have to version the build directory - which is not a common practice in the wild.

Love the idea. I had the same thoughts when looking at the new folder structure.

New blocks must be added to the src directory as would be done normally when using the scripts package.

What prevents us from keeping all types of blocks in src folder? example:

--| src
----| blocks-plain
----| blocks-jsx
----| other

An alternative would be to make the src folder customizable inside wp-scripts build.

Do you know why we need languages folders for blocks? I don't think it's necessary anymore when you publish your plugin to the Plugin Directory.

.nvmrc Outdated Show resolved Hide resolved
@gziolo
Copy link
Member

gziolo commented Mar 16, 2022

How changes proposed in #193 relate to this PR?

@ryanwelcher
Copy link
Contributor Author

How changes proposed in #193 relate to this PR?

I think that this PR takes precedence over #193 from a code perspective. Once this has landed, I would adjust #193 to introduce the linting actions as well as the build/release ones mentioned here.

@ryanwelcher
Copy link
Contributor Author

What prevents us from keeping all types of blocks in src folder? example:

This is tricky, for anything that contains a block.json file, everything works as expected and all the assets are copied correctly to the correct folders.

For the non-block examples, however, only the index.php is copied, unless we drop a block.json in there which will be confusing from the perspective of someone trying to learn. I would love to not have to use a custom webpack config here but not sure we can accomplish that without changing the @wordpress/scripts package to handle non-blocks assets the in the same way as block assets

@gziolo
Copy link
Member

gziolo commented Mar 17, 2022

An alternative would be to make the src folder customizable inside wp-scripts build.

Do you think we should try renaming src to blocks-jsx once wp-scripts is able to work with custom source folders?

Do you know why we need languages folders for blocks? I don't think it's necessary anymore when you publish your plugin to the Plugin Directory.

This needs to be confirmed.

Screenshot 2022-03-17 at 10 46 35

The current folder structure is a bit confusing when you see it for the first time. We need at least to document it because src might trick people in both ways:

  • they can think it's code for the plugin
  • they can think all examples are there

I think it's important to name top-level folders correctly and document them in the README file.

runs-on: ubuntu-latest
strategy:
matrix:
node-version: [10.x, 12.x, 14.x]
Copy link
Member

Choose a reason for hiding this comment

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

I recommend using Node.js 14, 16 and 18 - all current LTE 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.

This action is going to be changed but I'll update this now - thanks!

import { registerFormatType, toggleFormat } from '@wordpress/rich-text';
import { RichTextToolbarButton } from '@wordpress/block-editor';

const MyCustomButton = ({ isActive, onChange, value }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Code formatting looks like a regular Prettier instead of the wp-prettier fork 😅

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 can't even tell the difference any more 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Locally I added prettier": "npm:[email protected]", and ran format from wp-scripts which seemed to work so I pushed those changes update. I'm in a weird state where I have my eslint wanting one thing and format another so I'll have to get that sorted out.

@ryanwelcher
Copy link
Contributor Author

I've made a few changes to the approach for this PR:

  1. I've created separate build processes for the blocks and non-block examples. This seemed to make the most sense as I needed to add a custom webpack config for the non-blocks.
  2. I've renamed src to blocks-jsx and used the new --webpack-src-dir to point to the new directory and changed the non-jsx versions to be block-non-jsx
  3. I've added the wp-prettier alias as a dependency because of issues with the actually prettier package being installed and causing issues with formatting

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Let's give it a try and iterate further with follow-up work. It should help with repository management. Awsome work @ryanwelcher.

@ryanwelcher ryanwelcher merged commit 33348e9 into trunk May 11, 2022
@ryanwelcher ryanwelcher deleted the try/single-build-process branch May 11, 2022 14:18
welenofsky added a commit to welenofsky/gutenberg that referenced this pull request Jul 7, 2022
Fix the link to the finished app code in the gutenberg-examples repository to point to new location. See WordPress/gutenberg-examples@33348e9 or WordPress/gutenberg-examples#195
fabiankaegy pushed a commit to WordPress/gutenberg that referenced this pull request Jul 7, 2022
Fix the link to the finished app code in the gutenberg-examples repository to point to new location. See WordPress/gutenberg-examples@33348e9 or WordPress/gutenberg-examples#195
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.

4 participants