Skip to content

Conversation

@sejas
Copy link
Collaborator

@sejas sejas commented May 23, 2023

What?

Remove defineVirtualWpConfigConsts in favor of defineWpConfigConsts, adding a virtualize option.

Why?

Code optimization discussed here #343 (comment)

Testing Instructions

  1. Observe the blueprint tests pass
  2. Run nx preview wp-now start --path=/path/to/wordpress-plugin-or-theme
  3. Observe there is no playground-consts.json file added to the root of wordpress.
  4. Observe the wp-config.php was not modified.

@sejas sejas self-assigned this May 23, 2023
@sejas sejas requested a review from adamziel May 23, 2023 16:12
@adamziel
Copy link
Collaborator

adamziel commented May 23, 2023

Thank you @sejas! Something is wrong with the formatting, though. I added a husky pre-commit hook to run nx format but I guess it doesn't auto-install 🤔 any ideas how to fix that?

@sejas
Copy link
Collaborator Author

sejas commented May 24, 2023

Thank you @sejas! Something is wrong with the formatting, though. I added a husky pre-commit hook to run nx format but I guess it doesn't auto-install 🤔 any ideas how to fix that?

Fixed! 5944df3

@sejas
Copy link
Collaborator Author

sejas commented May 25, 2023

@adamziel, can I merge this branch?

@sejas
Copy link
Collaborator Author

sejas commented May 26, 2023

@adamziel, I would appreciate a final review of this PR. Thank you!

Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

Almost there, I only left a few nit picks. Thank you for working on this!

@adamziel adamziel merged commit 1387969 into WordPress:trunk May 26, 2023
@adamziel
Copy link
Collaborator

Thank you for consolidating that logic @sejas!

@sejas sejas deleted the update/refactpr-define-wp-config-consts-step branch May 31, 2023 09:20
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