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

Download exact WordPress version when passing an alias #324

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

zaguiini
Copy link
Contributor

@zaguiini zaguiini commented Jul 5, 2024

What?

Resolves to an actual WordPress version when starting wp-now with --wp=latest|trunk|nightly.

Why?

In #214, it was reported that running --wp=latest might start the server with an outdated version of WordPress because we're caching the version locally by the alias (which changes between published versions) and not the version number.

How?

Instead of having latest, trunk and nightly folders, we're resolving these aliases using the api.wordpress.org endpoint to actual versions, and caching the version number instead of the alias.

After confirmation this is the approach we want to move forward with, I'll add/update tests.

Testing Instructions

Build it locally and verify that following commands produce the following outputs:

Command CLI options output
start wp: 6.5.5 (resolved from alias: latest)
start --wp=latest wp: 6.5.5 (resolved from alias: latest)
start --wp=trunk wp: 6.7-alpha-xxxx (resolved from alias: trunk)
start --wp=nightly wp: 6.7-alpha-xxxx (resolved from alias: nightly)
start --wp=6.3 wp: 6.3

@zaguiini
Copy link
Contributor Author

zaguiini commented Jul 5, 2024

cc @sejas

Copy link
Collaborator

@sejas sejas left a comment

Choose a reason for hiding this comment

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

@zaguiini , thank you for creating the PR. It looks good, working as expected.

I was thinking if it would be possible to remove isDeveloperBuild in favor of a more simple check, like directly using the provided alias.

Let me know if you want me to update the tests as well.

Thank you!

I tested it and I confirm that after building the project npx nx build wp-now, I could run wp-now start with the following results:

node dist/packages/wp-now/cli.js start
node dist/packages/wp-now/cli.js start --wp=latest
It downloaded and used ~/.wp-now/wordpress-versions/6.5.5

node dist/packages/wp-now/cli.js start --wp=6.6-beta2
It downloaded and used ~/.wp-now/wordpress-versions/6.6-beta2

node dist/packages/wp-now/cli.js start --wp=trunk
node dist/packages/wp-now/cli.js start --wp=nightly
It downloaded and used ~/.wp-now/wordpress-versions/6.7-alpha-58716

Starting the server......
directory: /Users/macbookpro/proyectos/a8c/playground-tools
mode: playground
php: 8.0
wp: 6.7-alpha-58716 (resolved from alias: trunk)
Downloading WordPress 6.7-alpha-58716...
SQLite folder already exists. Skipping download.
Server running at http://localhost:8881

Passing an invented version returned a correct error:

node dist/packages/wp-now/cli.js start --wp=foo     

wp-now start

Start the server

Options:
      --version         Show version number                            [boolean]
      --path            Path to the PHP or WordPress project. Defaults to the cu
                        rrent working directory.                        [string]
      --php             PHP version to use.                             [string]
      --wp              WordPress version to use: e.g. '--wp=6.2'       [string]
      --port            Server port                                     [number]
      --blueprint       Path to a blueprint file to be executed         [string]
      --reset           Create a new project environment, destroying the old pro
                        ject environment.                              [boolean]
      --skip-browser    Do not launch the default browser
                                                      [boolean] [default: false]
      --inspect         Use Node debugging client.                      [number]
      --inspect-brk     Use Node debugging client. Break immediately on script e
                        xecution start.                                 [number]
      --trace-exit      Prints a stack trace whenever an environment is exited p
                        roactively, i.e. invoking process.exit().       [number]
      --trace-uncaught  Print stack traces for uncaught exceptions; usually, the
                         stack trace associated with the creation of an Error is
                         printed, whereas this makes Node.js also print the stac
                        k trace associated with throwing the value (which does n
                        ot need to be an Error instance).               [number]
      --trace-warnings  Print stack traces for process warnings (including depre
                        cations).                                       [number]
  -h, --help            Show help                                      [boolean]

Unrecognized WordPress version. Please use "latest" or numeric versions such as "6.2", "6.0.1", "6.2-beta1", or "6.2-RC1"

Comment on lines -41 to -45
if (!isValidWordPressVersion(version)) {
throw new Error(
'Unrecognized WordPress version. Please use "latest", "trunk", "nightly", or numeric versions such as "6.2", "6.0.1", "6.2-beta1", or "6.2-RC1"'
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@zaguiini
Copy link
Contributor Author

zaguiini commented Jul 15, 2024

I was thinking if it would be possible to remove isDeveloperBuild in favor of a more simple check, like directly using the provided alias.

It's possible. I just wanted to stick as much as possible to the "a function should do one thing" principle. Also if we move the alias resolver within downloadWordPress, a bigger refactor will be required so that the "resolved from alias" output part shows before the version is downloaded.

But let me know if you have other ideas.

Let me know if you want me to update the tests as well.

That'd be great 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants