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

Explore implementing @wordpress/scripts #118

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

g-elwell
Copy link
Member

@g-elwell g-elwell commented May 17, 2024

Description

This PR explores the possibility of extending our config from those exposed via @wordpress/scripts

Related #117

This approach retains the existing package discovery to determine which mode to run the build tools in, and which packages to build, but rather than stitching together webpack entrypoints for each project, it will use concurrently to run separate webpack processes for each project.

This greatly simplifies the configuration within the build tools, but is not yet fully tested/implemented.

Tasks to do:

Project discovery

  • Can detect mode and projects discover projects*
  • Works from single project mode (from a plugin or theme root)*
  • Works in all projects mode (from wp-content root with nested plugins/themes)*
  • Works in list mode (from wp-content root specifying which nested plugins/themes to build)*

Webpack config

  • Extends config from @wordpress/scripts
  • Detects src/entrypoints*
  • Copies /static directory content to/build/static
  • Generates JS bundles**
  • Generates CSS bundles**
  • block.json support
  • render.php support
  • Alias support (eg import myComponent from '@Components')*
  • Lint output shown in terminal*

ESLint config

  • Extends config from @wordpress/scripts
  • Overrides config with our own customisations (most notably, still using airbnb style guide)*
  • Can be further extended at project-level

Prettier config

  • Extends config from @wordpress/scripts
  • Overrides config with our own customisations*
  • Can be further extended at project-level

Stylelint config

  • Extends config from @wordpress/scripts
  • Overrides config with our own customisations*
  • Can be further extended at project-level

* build tools feature, not part of @wordpress/scripts
** change to existing build tools, bundles are outputted to /build not /dist/scripts and dist/styles

Next steps

  • Re-implement PostCSS config
  • Re-implement browserlist config
  • Confirm typescript support from wp-scripts, discuss whether we're opinionated on anything to override configs
  • Confirm all the configs can be extended at project level
  • Test linting works within VSCode and document any changes to this config
  • Consider exposing commands to run linting scripts outside of the build process?
  • Review configs for duplication/conflicts
  • Confirm concurrent build independent exit process doesn't cause issues on CI

@g-elwell g-elwell force-pushed the test/wp-scripts-concurrently branch from 2fd3747 to 37fc063 Compare May 17, 2024 20:12
configs/browserlist.js Show resolved Hide resolved
configs/tsconfig.json Show resolved Hide resolved
configs/typescript/images.d.ts Show resolved Hide resolved
index.js Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This might be the only plugin that I can still see a need for if its not handled by wp-scripts. This came from a need with developers including various files and how they were handled. A warning added to show them that a file should not be in that location and would need to be moved.

Is this situation handled by wp-scripts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for flagging this, wp-scripts doesn't have any functionality for supporting a static directory, so I carried over our implementation but missed this part

configs/webpack.config.js Show resolved Hide resolved
@ampersarnie
Copy link
Member

ampersarnie commented May 21, 2024

I've put in some additional time to test this further today and have come across a number of issues. I'll be listing as their own individual comments so they can be referenced properly.

I also want to re-iterate what was discussed in our strategy meeting: this is moving out of scope with additional changes to the way the. I want the focus to be on providing a @wordpress/scripts compatible solution, and we can address other build processes later. these changes are already distracting to this PR and what is trying to be achieved here, so concentration should be redirected back to focusing on wp-scripts itself.

Those additional changes should be in their own ticket with their own scope.

@ampersarnie
Copy link
Member

Build Not Able To Run

The build is not able to run without wp-scripts being present in the project setup. Upon looking into the code, this seems to be because the concurrently command is cding into a project directory and executing the given command (wp-script) from that directory, where the command is not available.

This means that @wordpress/scripts or @bigbite/build-tools will need to be setup and installed on each config. Creating issues for a monorepo setup.

rm -rf node_modules
npm i
cd example-site
node ../cli/cli.js build

Output

Compiling all projects in development mode.
Processing the following projects:
 * test-client-plugin [./client-mu-plugins/test-client-plugin/package.json]
 * standards [./plugins/standards/package.json]
 * test-plugin [./plugins/test-plugin/package.json]
 * test-theme [./themes/test-theme/package.json]

[test-client-plugin] Running build...
[standards] Running build...
[test-theme] Running build...
[test-plugin] Running build...
[test-client-plugin] /bin/sh: wp-scripts: command not found
[test-theme] /bin/sh: wp-scripts: command not found
[test-plugin] /bin/sh: wp-scripts: command not found
[standards] /bin/sh: wp-scripts: command not found
[test-client-plugin] echo "Running build..." && cd ./client-mu-plugins/test-client-plugin && wp-scripts start ./src/entrypoints/*.js --config /Users/paultaylor/Developer/work/BigBite/build-tools/configs/webpack.config.js exited with code 127
[test-theme] echo "Running build..." && cd ./themes/test-theme && wp-scripts start ./src/entrypoints/*.js --config /Users/paultaylor/Developer/work/BigBite/build-tools/configs/webpack.config.js exited with code 127
[standards] echo "Running build..." && cd ./plugins/standards && wp-scripts start ./src/entrypoints/*.js --config /Users/paultaylor/Developer/work/BigBite/build-tools/configs/webpack.config.js exited with code 127
[test-plugin] echo "Running build..." && cd ./plugins/test-plugin && wp-scripts start ./src/entrypoints/*.js --config /Users/paultaylor/Developer/work/BigBite/build-tools/configs/webpack.config.js exited with code 127
node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "[object Array]".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

Node.js v21.7.3

@ampersarnie
Copy link
Member

a11y package conflict

Running a successful build will show errors for eslint and the jsx-a11y.

Output

[test-client-plugin] ERROR in [eslint] Plugin "jsx-a11y" was conflicted between "../../package.json » ./../configs/eslint » eslint-config-airbnb » /Users/paultaylor/Developer/work/BigBite/build-tools/node_modules/eslint-config-airbnb/rules/react-a11y.js" and "BaseConfig » eslint-config-airbnb » /Users/paultaylor/Developer/work/BigBite/build-tools/node_modules/eslint-config-airbnb/rules/react-a11y.js".

@ampersarnie
Copy link
Member

--once flag throws errors

Running a build using the --once flag to not have the setup perform a watch (default behaviour), an error is thrown during compile after lint reporting. This will always produce an error, whether projects are defined or not.

Output

[test-plugin] echo "Running build..." && cd ./plugins/test-plugin && wp-scripts build ./src/entrypoints/*.js --config /Users/paultaylor/Developer/work/BigBite/build-tools/configs/webpack.config.js exited with code 1
node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "[object Array]".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

@ampersarnie
Copy link
Member

Linters catching files outside of src

When files exist outside of src they can get caught by the linter.

Output

[test-plugin] ERROR in [stylelint]
[test-plugin] dist/styles/editor.css
[test-plugin]  1:3    ✖  Expected whitespace after "/*"                          comment-whitespace-inside
[test-plugin]  2:337  ✖  Expected line length to be no more than 100 characters  max-line-length
[test-plugin] ERROR in [stylelint]
[test-plugin] styles.css
[test-plugin]  1:1   ✖  Unexpected unknown type selector "test"   selector-type-no-unknown
[test-plugin]  2:5   ✖  Expected indentation of 2 spaces          indentation
[test-plugin]  2:12  ✖  Unexpected named color "red"              color-named
[test-plugin]  3:1   ✖  Unexpected missing end-of-source newline  no-missing-end-of-source-newline

@ampersarnie
Copy link
Member

--quiet flag no longer producing reduced output

Previously the --quiet flag would produce a reduced output from the build process. This has been removed from the build command and produces a full complete output. The flag is also found as part of the command configuration.

@ampersarnie
Copy link
Member

Always runs in production mode

The webpack config always causes the script to run in production mode, even when a --production flag is not used.

Output of config from console.log

{
	mode: 'production',
	target: 'browserslist:/Users/paultaylor/Developer/work/BigBite/build-tools/node_modules/@wordpress/scripts/config/.browserslistrc',
	output: {
		filename: '[name].js',
		path: '/Users/paultaylor/Developer/work/BigBite/build-tools/example-site/plugins/test-plugin/build'
	}
}

@g-elwell
Copy link
Member Author

I also want to re-iterate what was discussed in our strategy meeting: this is moving out of scope with additional changes to the way the. I want the focus to be on providing a @wordpress/scripts compatible solution, and we can address other build processes later. these changes are already distracting to this PR and what is trying to be achieved here, so concentration should be redirected back to focusing on wp-scripts itself.

Those additional changes should be in their own ticket with their own scope.

Hi @ampersarnie, I understand your concerns. The reason I've left this PR in draft status is because I'm simply exploring an idea, it's not feature complete by any means and I'm also not necessarily aiming for it to be merged.

The reasons I shared it are twofold:

  1. To bring awareness to and get some input on the viability of running concurrently. This is fundamentally different to how the build tools works currently and I'm apprehensive about it as a result. My hope is to get this to a point where it's fully compatible with the existing build tools, despite working completely differently 'under the hood', but this may not be possible.
  2. To prevent any duplicate work, in case anyone else begins to go down a similar path in relation to [Feature]: Replace webpack and linting config with @wordpress/scripts #117

As I mentioned on the original issue, I first looked at extending the existing build-tools webpack config with support for wp-scripts but this proved quite difficult, hence I began looking into alternatives.

I'm not sure from your comments whether you want me to address the points you've raised or you're asking me to drop this approach entirely?

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