-
Notifications
You must be signed in to change notification settings - Fork 270
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
Blueprints and Client: update engine to node v20 on package.json
#591
Conversation
@sejas should we bump the version for all packages and also in the top-level I'm just wondering if there's any benefit to having different minimum versions in the different packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some JS classes such as
CustomEvent
andFile
that are implemented in node v20.
Would it be possible to shim these classes so we can keep node 16/18 support? If node 18 is LTS, I'm concerned that we'll lose some number of users by requiring node 20. It might break the VS Code Extension too.
@dmsnell, the benefit would be that the rest of the packages will work in third party apps with lower node versions. @adamziel asked to upgrade just those packages.
Maybe I understood it too literally. I have doubts if that engine section will be overwritten in the next release. |
@danielbachhuber, Yes, it's possible.
I took this approach since @adamziel suggested it over writing polyfills.
This week, I can estimate the level of effort to pull the NodeJS code and support lower versions. And we can decide if it's worth it or not. Sounds good? |
@sejas Sure, that's fine. |
This is pure personal preference, but I'm fairly okay with aggressively updating the minimum version and even more so of pinning all the versions together. It could be nice indicating a given package doesn't rely on a certain version, but then once people use the packages together they would need to update anyway. Also if we can avoid polyfills by bumping the minimum version then that's code we don't have to write and schedule to remove later when it's no longer needed because of some actual version requirement bump. This is still a fairly new and rapid project. We can make some more pressing demands on versions, plus if someone really wants to they can still run it in old versions, but at their own risk. The version information in |
@dmsnell I think this is at odds with "most users" being able to use See some conversation in WordPress/playground-tools#83. I think we should support LTS as a minimum if we care about other users being able to use both WordPress Playground and |
@danielbachhuber that's fine, which is why I added the caveat stating my personal preference. there are two issues in the quoted comment though: minimum version and harmonizing the versions. for the sake of integrations there is still potential value in making all packages require the same base version. it's obviously not a problem to specify separate ones, but it's still a tradeoff I think we are free to consider. |
Found out that the current version of VS Code bundles Node.js It's a somewhat old version that comes bundled with Electron, which lags behind until its Node.js runtime is merged from upstream. Currently, VS Code bundles Electron 22. Even the newest version of Electron 25 bundles Node.js https://releases.electronjs.org/ That means, as @danielbachhuber suggested earlier, the VS Code extension for the Playground needs to support Node.js 16/18 for the foreseeable future. As far as we know, the only thing necessary to meet that requirement is to polyfill the |
Oh yeah, I forgot about the VS Code Extension. @dmsnell and I didn't factor it into our conversation yesterday, whoops. Thanks for mentioning! |
Sounds good; let's just polyfill then. Thanks for noticing that @eliot-akira |
|
What is this PR doing?
Update the node version required for:
@wp-playground/blueprints
@wp-playground/client
What problem is it solving?
CustomEvent
andFile
that are implemented in node v20.How is the problem addressed?
Update the node version defined in
package.json
>engine
Testing Instructions