Skip to content

Migrating build process from laravel-mix to wp-scripts#341

Open
sabbir1991 wants to merge 4 commits intodevelopfrom
fix/migrate-laravel-mix-to-wp-scripts
Open

Migrating build process from laravel-mix to wp-scripts#341
sabbir1991 wants to merge 4 commits intodevelopfrom
fix/migrate-laravel-mix-to-wp-scripts

Conversation

@sabbir1991
Copy link
Member

Related issue: #330

Copilot AI review requested due to automatic review settings March 6, 2026 09:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Migrates the plugin’s non-block asset pipeline from laravel-mix to @wordpress/scripts (wp-scripts) to address dependency/security issues (Issue #330) while keeping the existing block build workflow.

Changes:

  • Replaced laravel-mix build steps with a custom webpack.assets.config.js based on @wordpress/scripts.
  • Updated PHP asset registration/enqueue logic and related unit tests to match new generated asset filenames/paths.
  • Removed laravel-mix/cross-env dependencies and updated npm scripts accordingly.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
webpack.mix.js Removes the old Laravel Mix build configuration.
webpack.assets.config.js Adds wp-scripts-based webpack config for non-block JS/CSS outputs.
assets/src/js/login.js Imports SCSS so wp-scripts generates the corresponding CSS during bundling.
src/Modules/Assets.php Updates registered stylesheet path to the new wp-scripts-generated CSS filename/location.
src/Modules/OneTapLogin.php Switches OneTap script URL to the new (non-.min) build output name.
tests/php/Unit/Modules/AssetsTest.php Updates expectations for the new CSS path/media argument.
package.json Updates build/watch scripts to use wp-scripts for both assets and blocks; removes Mix deps.
package-lock.json Reflects dependency graph changes after removing Mix/cross-env and updating scripts.
Comments suppressed due to low confidence (1)

src/Modules/OneTapLogin.php:157

  • filemtime() is called directly on assets/build/js/onetap.js for the script version. If the built asset is missing (e.g., dev install without running the build, or a packaging mistake), this will emit a PHP warning. Consider guarding with file_exists() (or reusing the Assets module’s safe version resolution approach) and falling back to a stable plugin version when the file is absent.
			trailingslashit( plugin()->url ) . 'assets/build/js/onetap.js',
			[
				'wp-i18n',
			],
			filemtime( trailingslashit( plugin()->path ) . 'assets/build/js/onetap.js' ),
			true

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...( defaultConfig.output || {} ),
path: path.resolve( __dirname, 'assets/build/js' ),
filename: '[name].js',
clean: false,
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

output.clean is explicitly disabled. This can leave stale JS/CSS/asset-module outputs in assets/build/js across builds (especially if chunking or asset filenames change), which can make debugging harder and potentially ship outdated artifacts. If this directory is dedicated to these assets, consider enabling cleaning (or removing this override) and/or ensuring other build outputs go to separate directories.

Suggested change
clean: false,
clean: true,

Copilot uses AI. Check for mistakes.
@bhavz-10
Copy link

bhavz-10 commented Mar 9, 2026

Hello @sabbir1991

I think we should keep the file structure as it was earlier, having the css files inside the build/css directory and not move them to the js inside the build dir. Can you confirm if this change was deliberate?

Thanks 🙌

@sabbir1991
Copy link
Member Author

I didn't change the CSS build path. Since it doesn't affect any improvement. I just reduced the complexity from the webpack.assets.config.js file and keep simple build process as like gutenberg block. If we want we can do that later as well

*/
public function register_login_styles(): void {
$this->register_style( self::LOGIN_BUTTON_STYLE_HANDLE, 'build/css/button/style.css' );
$this->register_style( self::LOGIN_BUTTON_STYLE_HANDLE, 'build/js/style-login.css' );
Copy link

Choose a reason for hiding this comment

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

This has changed from css to js.

@bhavz-10
Copy link

bhavz-10 commented Mar 9, 2026

I didn't change the CSS build path. Since it doesn't affect any improvement. I just reduced the complexity from the webpack.assets.config.js file and keep simple build process as like gutenberg block. If we want we can do that later as well

https://github.com/rtCamp/login-with-google/pull/341/changes#r2905350393

This is what I was talking about

Copy link

@bhavz-10 bhavz-10 left a comment

Choose a reason for hiding this comment

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

LGTM ✔️

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.

3 participants