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

New compilation process #24

Open
wants to merge 33 commits into
base: feature/modernization
Choose a base branch
from

Conversation

dpanta94
Copy link

@dpanta94 dpanta94 commented Oct 12, 2022

Changed assets directory structure from assets to src. Also included directories for images and fonts. All of these will be moved into a directory named assets, keeping their directory structure during npm run build.

Fixed hook plugin_name_loaded which was being called twice.

Fiixed src of the admin assets.

Pending:

  • blocks consideration
  • use script dependencies produced by wp-scripts when using wp_register_script or wp_enqueue_script
  • npm run archive, produces a zip that includes only the necessary files of the project. Need to make it, to also include all these files within a directory named "plugin-name"

To test the changes

  • checkout current branch
  • npm install
  • execute the scripts declared in package.json that aren't prefixed with pre or post

@dpanta94 dpanta94 changed the title changed dir structure for assets New compilation process Oct 12, 2022
@dpanta94 dpanta94 requested a review from msaggiorato October 12, 2022 13:51
@dpanta94 dpanta94 self-assigned this Oct 12, 2022
@dpanta94 dpanta94 marked this pull request as ready for review October 12, 2022 13:51
Copy link
Member

@msaggiorato msaggiorato left a comment

Choose a reason for hiding this comment

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

Leaving some comments here

plugin-name/assets/index.php Show resolved Hide resolved
plugin-name/i18n/languages/plugin-name.pot Outdated Show resolved Hide resolved
plugin-name/includes/Admin/Assets.php Outdated Show resolved Hide resolved
plugin-name/includes/Main.php Show resolved Hide resolved
plugin-name/package.json Outdated Show resolved Hide resolved
plugin-name/package.json Show resolved Hide resolved
module.exports = {
...defaultConfig,
entry: getWebpackEntryPoints(),
output: {
Copy link
Member

Choose a reason for hiding this comment

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

Are we producing both minified and unminified versions in parallel here?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure about the parallel term, but for JS files we are producing minified and unminifiied. For CSS files we are only producing minifiied versions.

Copy link
Member

Choose a reason for hiding this comment

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

What i meant was to run these in the same build process (not having to run it twice)

Copy link
Author

@dpanta94 dpanta94 Oct 13, 2022

Choose a reason for hiding this comment

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

Yes, normal/min JS and min CSS are being produced within the same npm run build 😄

Copy link
Author

Choose a reason for hiding this comment

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

@msaggiorato, also producing minifiied and not versions of the css files. Only drawback is that i could not find a way to achieve that without overwritting all the webpack.module.rules from the original wordpress-scripts webpack.confiig.js file.
Of course, ive brought all the needed rules in our webpack.config.js file.

Copy link
Author

Choose a reason for hiding this comment

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

@msaggiorato i think this comment can be resolved. Your commits are fixing this 😄

@dpanta94
Copy link
Author

@msaggiorato looks really good! Ive added a postbuild command in order to produce a zip ready to be uploaded.

"makepot": "wpi18n makepot --domain-path=\"i18n/languages\" --pot-file=\"plugin-name.pot\" --type=\"plugin\" --main-file=\"plugin-name.php\" --exclude=\"node_modules,tests,docs,docker,vendor\"",
"watch": "webpack --mode=none --watch",
"prearchive": "rm -rf $npm_package_config_wp_org_slug.zip vendor/ node_modules/ && composer install --no-dev --optimize-autoloader && npm install --only=prod --ignore-scripts",
"archive": "composer archive --file=$npm_package_config_wp_org_slug --format=zip",
"archive": "cross-var rm -rf $npm_package_config_wp_org_slug.zip vendor/ node_modules/ && composer install --no-dev --optimize-autoloader && npm install --only=prod --ignore-scripts && composer archive --file=$npm_package_config_wp_org_slug --dir=$npm_package_config_wp_org_slug --format=zip",
Copy link
Member

Choose a reason for hiding this comment

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

rm -rf still is dependant on a unix based shell right?

"postbuild:assets": "rm -rf assets/css/**/*.php assets/css/**/*.js",
"build": "npm run makepot && npm run build:assets && npm run archive"
"build": "npm run makepot && npm run build:assets && npm run archive",
"postbuild": "cross-var unzip -q $npm_package_config_wp_org_slug/$npm_package_config_wp_org_slug.zip -d $npm_package_config_wp_org_slug && rm $npm_package_config_wp_org_slug/$npm_package_config_wp_org_slug.zip && zip -q -r $npm_package_config_wp_org_slug.zip $npm_package_config_wp_org_slug -qq && rm -rf $npm_package_config_wp_org_slug"
Copy link
Member

Choose a reason for hiding this comment

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

unzip itself is still dependant on a unix shell right?

],
},
plugins: [
new RemoveEmptyScriptsPlugin(),
Copy link
Member

Choose a reason for hiding this comment

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

Is this an indenting error?

@dpanta94 dpanta94 removed their assignment Oct 29, 2024
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