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

wp-now: Error if node doesn't meet the minimum required version #434

Merged
merged 1 commit into from
May 25, 2023

Conversation

danielbachhuber
Copy link
Member

Fixes #429

What?

Exits with an error if node doesn't meet the minimum required version.

$ wp-now
You are running Node.js version 14, but this application requires at least Node.js 18. Please upgrade your Node.js version.

Why?

Without this, wp-now fails cryptically and provides no context for how it fails.

Testing Instructions

TBD

@danielbachhuber
Copy link
Member Author

@adamziel How do I test the built version?

Here's what I tried:

$ npm run build
$ node /Users/danielbachhuber/projects/wordpress-playground/dist/packages/wp-now/main.js
internal/process/esm_loader.js:74
    internalBinding('errors').triggerUncaughtException(
                              ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '/Users/danielbachhuber/projects/wordpress-playground/node_modules/@php-wasm/node/' imported from /Users/danielbachhuber/projects/wordpress-playground/dist/packages/wp-now/main.js

@danielbachhuber danielbachhuber changed the title wp-now: Error if node doesn't meet the minimum reqiured version wp-now: Error if node doesn't meet the minimum required version May 24, 2023
@adamziel
Copy link
Collaborator

@adamziel How do I test the built version?

nx preview wp-now should work.

But wait:

  • Could this use the engines feature of package.json instead of adding a custom condition?
  • Let's find a way of addressing this in all the packages and not just wp-now. The engines entry could perhaps do it with little effort.

@danielbachhuber
Copy link
Member Author

nx preview wp-now should work.

@adamziel Er, but then I'd be using nx, etc. etc. I want to test wp-now as though an end user has installed it.

Could this use the engines feature of package.json instead of adding a custom condition?

ChatGPT says it's not quite the same effect:

image

Let's find a way of addressing this in all the packages and not just wp-now. The engines entry could perhaps do it with little effort.

Are other packages run at the command line? How would we test this?

@adamziel
Copy link
Collaborator

Er, but then I'd be using nx, etc. etc. I want to test wp-now as though an end user has installed it.

Ah, I see. That's going to be tough in the current setup. It needs to import dependencies that don't live in node_modules and nx sets up a custom loader to enable that. Here's a different idea: build wp-now, the binary, as a single bundle and avoid importing things at all.

ChatGPT says it's not quite the same effect

It doesn't have the same effect but it is the standard way of solving this problem for a developer working with the repository clone.

The problem with checking node version is that it won't work if you run wp-now outside of Node. Like in Deno, or in the VS Code extension, or (probably) in Stackblitz.

As for someone including wp-now in their project dependencies, that sounds like a node problem, not wp-now problem. The issue is with a missing Event class – maybe they shimmed that? Why prevent them from using a package they were able to install?

Are other packages run at the command line? How would we test this?

It's php-wasm/cli. I'm a bit confused, though – is this a bundling issue or a runtime issue? I thought the build command failed.

@danielbachhuber
Copy link
Member Author

I'm a bit confused, though – is this a bundling issue or a runtime issue? I thought the build command failed.

This is fixing the issue identified in WordPress/playground-tools#11, which originated from npm install -g @wp-now/wp-now.

Here's a different idea: build wp-now, the binary, as a single bundle and avoid importing things at all.

This seems like a separate issue...

It doesn't have the same effect but it is the standard way of solving this problem for a developer working with the repository clone.

Maybe we should do both?

@adamziel
Copy link
Collaborator

This is fixing the issue identified in WordPress/playground-tools#11, which originated from npm install -g @wp-now/wp-now.

Thanks for clarifying! From the stack trace, that's a @php-wasm/node issue. I don't want to throw an error in there, though. You can do that in wp-now, just keep in mind that will prevent non-node runtimes from using wp-now.

This seems like a separate issue...

It is a separate issue. I'm just answering the question of how to run it like a npm install -g user would. This repository isn't structured in a way that allows you to do that at the moment.

Maybe we should do both?

I wouldn't – Node is just one way of running JavaScript. Logging a warning without exiting could be a good middle-ground here.

@danielbachhuber
Copy link
Member Author

Per our conversation, we'll add engines in a follow-up PR.

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.

Make the minimal Node.js version clear
2 participants