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

Add WP version comparison script #68

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

swissspidy
Copy link
Collaborator

Ports over the GitHub action and the manual Bash script from https://github.com/swissspidy/compare-wp-performance

The individual package.json files are unfortunately necessary right now because of WordPress/gutenberg#51419

Copy link
Collaborator

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @swissspidy, this looks great overall! A few minor comments. I wasn't able to complete the process locally due to an error.

tools/compare-wp-performance/run.sh Show resolved Hide resolved
"homepage": "https://github.com/swissspidy/compare-wp-performance#readme",
"type": "module",
"devDependencies": {
"@wordpress/env": "^8.4.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we alternatively use an older version that doesn't have the bug and thus doesn't require the patch and extra package.json workaround?

Copy link
Collaborator Author

@swissspidy swissspidy Aug 11, 2023

Choose a reason for hiding this comment

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

I will need to check what‘s possible / where the bug was introduced and if there are features we can live without or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The bug has since been resolved, so we can use a newer version without a patch, but it still requires 2 package.json files. Or we leverage a global install.

tools/compare-wp-performance/old/package.json Show resolved Hide resolved
tools/compare-wp-performance/scripts/results.js Outdated Show resolved Hide resolved
@spacedmonkey spacedmonkey self-requested a review September 4, 2023 11:38
@felixarntz
Copy link
Collaborator

@swissspidy I realize this somehow fell off the radar, but was wondering whether we could pick it up again. I think it's close, could you follow up on #68 (comment) and see whether there are any new changes in your repo that should be ported into this PR when you get a chance?

@swissspidy
Copy link
Collaborator Author

Yeah I'll need to revisit this and merge in the latest changes. But holding off a bit now because it's not urgent (for 6.4 we can still use the existing repo) and there might be significant changes needed when tackling all the variance issues.

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