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.

Sign up for GitHub

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

Preferentially Execute Local wp-env #50980

Merged
merged 7 commits into from
May 26, 2023
Merged

Preferentially Execute Local wp-env #50980

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ccef1f7
Preferentially Execute Local `wp-env`
ObliviousHarmony May 25, 2023
f44e63a
Changelog
ObliviousHarmony May 25, 2023
a06252f
Documentation Update
ObliviousHarmony May 25, 2023
9dbf5e2
Fixed Version Loading
ObliviousHarmony May 25, 2023
32890b1
Removed Unnecessary Path Resolution
ObliviousHarmony May 25, 2023
b1baba3
Update packages/env/README.md
ObliviousHarmony May 26, 2023
c359fd4
Replaced Local Path Resolution
ObliviousHarmony May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/env/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### New feature

- Execute the local package's `wp-env` instead of the globally installed version if one is available.

## 8.0.0 (2023-05-24)

### Breaking Change
Expand Down
4 changes: 2 additions & 2 deletions packages/env/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ If your project already has a package.json, it's also possible to use `wp-env` a
$ npm i @wordpress/env --save-dev
```

At this point, you can use the local, project-level version of wp-env via [`npx`](https://www.npmjs.com/package/npx), a utility automatically installed with `npm`.`npx` finds binaries like wp-env installed through node modules. As an example: `npx wp-env start --update`.
If you have also installed `wp-env` globally, running it will automatically execute the local, project-level package. You can also execute it via [`npx`](https://www.npmjs.com/package/npx), a utility automatically installed with `npm`.`npx` finds binaries like `wp-env` installed through node modules. As an example: `npx wp-env start --update`.
ObliviousHarmony marked this conversation as resolved.
Show resolved Hide resolved

If you don't wish to use `npx`, modify your package.json and add an extra command to npm `scripts` (https://docs.npmjs.com/misc/scripts):
If you don't wish to use the global installation or `npx`, modify your `package.json` and add an extra command to npm `scripts` (https://docs.npmjs.com/misc/scripts):

```json
"scripts": {
Expand Down
25 changes: 23 additions & 2 deletions packages/env/bin/wp-env
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,25 @@
#!/usr/bin/env node
'use strict';
const command = process.argv.slice( 2 );
require( '../lib/cli' )().parse( command.length ? command : [ '--help' ] );

/**
* External dependencies.
*/
const path = require( 'path' );
const fs = require( 'fs' );

// Remove 'node' and the name of the script from the arguments.
let command = process.argv.slice( 2 );
// Default to help text when they aren't running any commands.
if ( ! command.length ) {
command = [ '--help' ];
}

// Rather than just executing the current CLI we will attempt to find a local version
// and execute that one instead. This prevents users from accidentally using the
// global CLI when a potentially different local version is expected.
const localPath = path.resolve( './node_modules/@wordpress/env/lib/cli.js' );
Copy link
Member

Choose a reason for hiding this comment

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

What if we use require.resolve( '@wordpress/env' ) instead? I think this will throw an error if the module can't be found, in which case we could revert to ../lib/cli.js. In my testing from a nested directory in a project, this is the result:

> require.resolve('@wordpress/env')
'/Users/noahallen/source/wp-calypso/node_modules/@wordpress/env/lib/env.js'

(And then we can change env to cli.)

require.resolve should handle more cases because it's using the Node module resolution algorithm, whereas the current approach probably won't fork in child directories

Copy link
Contributor Author

@ObliviousHarmony ObliviousHarmony May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this will throw an error if the module can't be found, in which case we could revert to ../lib/cli.js

Using path.resolve() in this case will just resolve an absolute path relative to process.cwd(). Since all we're doing is creating a path string, the fs.existsSync( localPath ) check below will protect us from trying to do something with a module that doesn't exist.

An interesting point though is that perhaps it might make sense be more explicit about what we're doing and use path.join( process.cwd(), 'node_modules/@wordpress/env/lib/cli.js? I'm going to make that change.

require.resolve should handle more cases because it's using the Node module resolution algorithm, whereas the current approach probably won't fork in child directories

I would worry that require.resolve is too heavy-handed. Node's module resolution algorithm includes searching the current directory and every parent directory for a module in a node_modules directory. Imagine that a package has a .wp-env.json built with a global version of X in mind, but, it's in a directory with a project at a higher-level that (for whatever reason) has a different wp-env version.

Since wp-env commands only care about the current working directory, I think it makes sense for this override to only apply in the current one as well. For instance, even though npx wp-env start will find the root instance if ran in a subdirectory, the command won't actually work because there's no package there.

Ultimately the question here is whether or not the user should expect it to find the current directory's package, or, if it should find the nearest one (traversing to the global install if it can't find one). Given that this is the typical behavior of NPM commands though, you might be right.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, you make a good point about wp-env being directory based. However, this makes me think of another scenario: monorepos! For example, I have a plugin in a monorepo -- the .wp-env.json file is under a path like ./apps/this-plugin. But node modules for that plugin would be stored in ./node_modules (since yarn will put all monorepo deps in the same place.) While I need to run .wp-env from the apps/plugin directory, this new feature wouldn't work unless we used require.resolve.

Imagine that a package has a .wp-env.json built with a global version of X in mind, but, it's in a directory with a project at a higher-level that (for whatever reason) has a different wp-env version.

Also a good point, but it's important to have explicit dependencies if you really need to depend on a specific version!

Copy link
Member

Choose a reason for hiding this comment

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

I think I agree with your last line, that the expected behavior is probably to resolve the current project's wp-env copy from node_modules, even if it's in a different directory. (And while it would be unexpected to resolve from outside the project in a parent directory, that doesn't seem like a common use case)

const cliPath = fs.existsSync( localPath ) ? localPath : '../lib/cli.js';
const cli = require( cliPath )();

// Now we can execute the CLI with the given command.
cli.parse( command );
5 changes: 5 additions & 0 deletions packages/env/lib/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const { execSync } = require( 'child_process' );
/**
* Internal dependencies
*/
const pkg = require( '../package.json' );
const env = require( './env' );
const parseXdebugMode = require( './parse-xdebug-mode' );
const {
Expand Down Expand Up @@ -110,6 +111,10 @@ module.exports = function cli() {
'populate--': true,
} );

// Since we might be running a different CLI version than the one that was called
// we need to set the version manually from the correct package.json.
yargs.version( pkg.version );

yargs.command(
'start',
wpGreen(
Expand Down