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

Replace gulp build with wp-scripts. #916

Merged
merged 42 commits into from
Aug 18, 2022

Conversation

peterwilsoncc
Copy link
Collaborator

@peterwilsoncc peterwilsoncc commented Aug 1, 2022

Description of the Change

Replace gulp and various config files with @wordpress/scripts.

I think this is ready for review, it should probably get double sign-off as it is quite large. I recommend reviewing this with whitespace suppressed.

I have put together a second webpack config for the npm run release command as I think it is needed to copy files in to the release subdirectory for committing to the stable branch. As noted inline, I would love an alternative suggestion as it doesn't seem ideal.

Closes #892.

How to test the Change

  • Ensure production file names remain the same
  • Test the various builds/npm scripts have effectively the same result (ignoring whitespace, variable munging differences)

Changelog Entry

Changed - Build process now uses @wordpress/scripts in place of Gulp.

Credits

Props @peterwilsoncc, @dinhtungdu

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

This removes all of the previous build tools and introduces the `@wordpress/scripts` as the build tool. This is intended to increase external contributions by using a tools known within the wider WordPress community.

As a WIP, the build is expected to be broken after this commit.
@peterwilsoncc peterwilsoncc self-assigned this Aug 1, 2022
@peterwilsoncc peterwilsoncc added this to the 2.0.0 milestone Aug 1, 2022
@peterwilsoncc peterwilsoncc force-pushed the fix/892-update-build-scripts branch from d30e9ab to 55d7975 Compare August 2, 2022 05:02
@@ -0,0 +1,35 @@
const CopyPlugin = require( 'copy-webpack-plugin' );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using this in place of the gulp copy script to copy all the files over during the release step.

I don't really like it or think it's a proper use of webpack so suggestions on how to do this without this would be much appreciated.

@peterwilsoncc peterwilsoncc force-pushed the fix/892-update-build-scripts branch from e8ce036 to eb89066 Compare August 2, 2022 05:19
@peterwilsoncc peterwilsoncc marked this pull request as ready for review August 2, 2022 05:54
@peterwilsoncc peterwilsoncc requested a review from dkotter August 2, 2022 05:54
# Conflicts:
#	.github/workflows/lint.yml
#	package-lock.json
#	package.json
Distributor now uses Cypress for e2e tests so the wp-scripts are unused. Existing workflows call phpunit directly so there is no need for the unit test script either.
@peterwilsoncc peterwilsoncc force-pushed the fix/892-update-build-scripts branch from c36ff84 to 6ddb027 Compare August 4, 2022 02:49
@peterwilsoncc peterwilsoncc force-pushed the fix/892-update-build-scripts branch from 6ddb027 to 5d94b4b Compare August 4, 2022 02:55
@peterwilsoncc
Copy link
Collaborator Author

I think I've addressed the various issues @dinhtungdu picked up:

  • fc21fd1 fixes the block editor -- the svg needs to be a react component now
  • 104ba23 fixes the name of the CSS file restoring admin-pull-table.min.css
  • 4cd3fc8 fixes the external connections screen

WP Scripts doesn't seem to produce code compatible with older versions of WP so I've pulled in 04a1580 from the requirements bump PR as both the version bump and this issue are on the same milestone.

@dinhtungdu
Copy link
Contributor

@peterwilsoncc Thanks for the updates! I can confirm all issues I noted are addressed.

I did another round of testing and found some additional issues, please check the screenshots below for more detail.

Screenshots Screen Shot 2022-08-06 at 00 04 43 Screen Shot 2022-08-06 at 00 03 20

@peterwilsoncc
Copy link
Collaborator Author

Distributor panel restored in 0b53814 -- I'd reversed a media query.

For the checked attribute showing, I think that's expected due to the differences between properties and attributes.

The property shows the current state, the attribute the state at page load. If you run jQuery('#dt-as-draft').prop( 'checked' ) in the browser console, you should get an accurate result when unchecked.

@peterwilsoncc
Copy link
Collaborator Author

@dinhtungdu I think this is ready for another review. I recommend suppressing white space.

For now I have kept all the built asset file names the same which means the webpack config is a little odd. I've done this for back-compat but if it's decided it isn't needed then that can be done in a smaller follow up commit.

Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉
Thank you so much for this hard work! This PR tested well on my end.

@peterwilsoncc peterwilsoncc merged commit d0031aa into develop Aug 18, 2022
@peterwilsoncc peterwilsoncc deleted the fix/892-update-build-scripts branch August 18, 2022 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Tooling: Replace custom gulp build with WP Scripts
2 participants