Preferentially Execute Local wp-env
#50980
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of always running the chosen version of `wp-env` we are going to try and find a local version in the current directory. This prevents the global `wp-env` from being used when a different local version is expected.
|
Size Change: -19.2 kB (-1%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
noahtallen
reviewed
May 25, 2023
packages/env/bin/wp-env
Outdated
| // 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' ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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)
Co-authored-by: Noah Allen <noahtallen@gmail.com>
This changes the path resolution to use Node's module resolution algorithm to be consistent with other usages of Node CLI tools.
|
Flaky tests detected in c359fd4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5094375396
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This pull request causes
wp-envcommands to be executed by the local package's version if one is available.Why?
When someone has a global
wp-envthat is a different version than the one installed in the package, runningwp-envcommands can be problematic. There may be compatibility problems for instance.How?
Rather than relying on something like
execSync, this PR loads thelib/cli.jsfile from the local package if it is available. This makes the entire execution seamless.Testing Instructions
versioninpackages/env/package.jsonto20.0.0.npm packinpackages/env.npm -g I ./wordpress-env-20.0.0.tgzand install this package globally.versionback to what it was before.wp-env --versioninpackages/env/liband confirm it is20.0.0.0.wp-env --versioninpackages/envand confirm it is the local version, probably8.0.0.