Allow current node LTS 14.17.6 #20990

guarani · 2021-09-07T17:31:10Z

Changes proposed in this Pull Request:

This change proposes allowing use of node ^14.17.6 (14.17.6 is the current node LTS version at time of writing) in addition to node 16.7.0 which is what the project currently specifies in its node engines attribute. .nvmrc will still point to 16.7.0, but the project can now be used in environments such as Gutenberg Mobile which are using node 14.17.6 LTS. This change is related to Bump Jetpack submodule ref to fix node compatibility issue wordpress-mobile/gutenberg-mobile#3928.

Jetpack product discussion

See p9dueE-3ls-p2#comment-6040

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Verify all required PR checks pass (this verifies that node 16.7.0 support is still working)
To verify node 14.17.6 support:
Checkout this branch locally
Switch to node 14.17.6 e.g. nvm use 14.17.6
Setup the environment (e.g. npm install -g pnpm && pnpm install && pnpm cli-setup && jetpack install --root) and verify it using tools/check-development-environment.sh
Build all affected packages and verify there are no errors:

jetpack build github-actions/repo-gardening \
jetpack build github-actions/required-review \
jetpack build js-packages/api \
jetpack build js-packages/base-styles \
jetpack build js-packages/components \
jetpack build js-packages/connection \
jetpack build js-packages/eslint-changed \
jetpack build js-packages/storybook \
jetpack build packages/connection-ui \
jetpack build packages/identity-crisis \
jetpack build packages/jitm \
jetpack build packages/lazy-images \
jetpack build plugins/backup \
jetpack build plugins/boost \
jetpack build plugins/jetpack \
jetpack build plugins/search

test-case-reminder

Here are some suggested test cases for this PR.

Connection

In-place connection with free plan
In-place connection with paid plan
In-place connection with product purchase
Classic connection. Use Safari, or set a constant JETPACK_SHOULD_NOT_USE_CONNECTION_IFRAME to true
Disconnect/reconnect connection
Secondary user connection
Connection on multisite

Verify that the changes are compatible with the plugins that use the connection package.

WooCommerce Payments
Jetpack Boost
Previous versions of Jetpack

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

matticbot · 2021-09-07T17:31:18Z

Caution: This PR has changes that must be merged to WordPress.com
Hello guarani! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D66469-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

package.json

This change proposes allowing use of node 14.17.6 (current node LTS version at time of writing) in addition to node 16.7.0 which is what the project currently specifies in its node engines attribute. `.nvmrc` will still point to 16.7.0, but the project can now be used in environments such as Gutenberg Mobile which are using node 14.17.6 LTS. Co-authored-by: David Calhoun <[email protected]>

github-actions · 2021-09-08T01:35:45Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: October 5, 2021.
Scheduled code freeze: September 28, 2021.

Backup plugin:

Next scheduled release: October 5, 2021.
Scheduled code freeze: September 27, 2021.

Boost plugin:

Next scheduled release: October 5, 2021.
Scheduled code freeze: September 27, 2021.

Search plugin:

Next scheduled release: October 5, 2021.
Scheduled code freeze: September 27, 2021.

guarani · 2021-09-08T02:47:24Z

Summary of failing tests:

Tests / PHP tests: PHP 7.4 WP master (pull_request): This failure includes the message "You need to run composer update before running the tests." and I see it failing on separate PRs (example, example), so this failure doesn't look related to this PR.
Linting / Changelogger validity (pull_request): I don't see any information about this error except for a nondescript "Error: Process completed with exit code 1.", and I don't see this error on other PRs, so not sure how to fix this yet.
wpcom — Tests failed: D66469-code: Again I don't see info on this so will have to dig further.

anomiex · 2021-09-08T14:39:01Z

Looks good to me, other than the wpcom failure. Which looks like more work to deal with.

You can check the TeamCity test on the Phabricator diff to find out more. The failure may be related to the version of Node available on WordPress.com.

Looks like the TeamCity job winds up combining the two version numbers when trying to decide which version to use, and so fails. Someone will have to get devops to update their script somehow.

Maybe we should start syncing .nvmrc or .github/versions.sh so they can read a usable version number more directly.

guarani · 2021-09-08T15:10:39Z

Looks like the TeamCity job winds up combining the two version numbers when trying to decide which version to use, and so fails.

@anomiex, thank you for spotting this! @dcalhoun and I were also just looking into this TeamCity job error and agree that it looks like the current setup to install node doesn't work well with the node engine being defined as ^14.17.6 || ^16.7.0. Perhaps it only works with a single version definition.

Maybe we should start syncing .nvmrc or .github/versions.sh so they can read a usable version number more directly.

I agree that it seems ideal to have the node version read from one of those places.

Someone will have to get devops to update their script somehow.

@anomiex, would you know the right person or team who could help us update this?

anomiex · 2021-09-08T16:27:17Z

Someone will have to get devops to update their script somehow.

@anomiex, would you know the right person or team who could help us update this?

Devops. I went ahead and created D66560-code and p3topS-VB-p2 to get it started.

guarani · 2021-09-08T17:15:48Z

Devops. I went ahead and created D66560-code and p3topS-VB-p2 to get it started.

Great! Thank you for doing that 🙇.

guarani · 2021-09-09T17:24:32Z

FYI: D66560-code is now closed, so I restarted the TeamCity job and it passed! 🎉 The CI check didn't seem to update though so I've re-triggered a build by pushing an empty commit. If all checks pass I'll mark this as ready for review.

guarani · 2021-09-09T18:14:53Z

All tests passed (the Tests / PHP tests: PHP 7.4 WP master (pull_request) failure is known and expected).

anomiex · 2021-09-09T18:16:18Z

Yeah, the CI doesn't always update right. Most often it's because it only reacts to the build of the diff that it pushed itself, not the build of the most recent diff.

guarani · 2021-09-09T18:34:48Z

@anomiex, thanks for your PR approval here!

From my side, this PR is ready, but I'm not sure if I need to get D66469-code approved first, so I added you as a reviewer (please feel free to guide me if there's someone else I should ask).

Also, I don't know if I should add the [Status] Needs Team Review or [Status] Needs Review. For context, I'm a Gutenberg Mobile dev so not sure if the team review label applies here. Also, I'm not sure if someone needs to add the [Status] Ready To Merge label.

anomiex · 2021-09-09T18:46:25Z

but I'm not sure if I need to get D66469-code approved first

Normally we consider the approval here to cover those as well.

Also, I don't know if I should add the [Status] Needs Team Review or [Status] Needs Review.

[Status] Needs Review is the one you would use to request a review from Crew, which is normally what's needed in order to merge the PR.

[Status] Needs Team Review is for if your team is working on something, your team can search for PRs with the right component's label plus that label. It might also be useful if your PR only touches Boost, Backup, or one of the other things where a non-Crew team has approval rights.

Also, I'm not sure if someone needs to add the [Status] Ready To Merge label.

It's not necessary, but I should have added it when I approved the PR. I just forgot. 😬

guarani · 2021-09-09T18:48:23Z

Gotcha, makes sense. I've merged now 🎉 🙌

github-actions · 2021-09-09T18:48:29Z

Great news! One last step: head over to your WordPress.com diff, D66469-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

jeherve · 2021-09-10T15:49:09Z

r231570-wpcom

The minimum version set in package.json is like `^14.18.3 || ^16.13.2`. We weren't handling that correctly, we were comparing with a minimum "version" of "14.18.3 || ^16.13.2" rather than anything well-formed. This changes the check to extract the constraint matching the current major version of node (i.e. "14.18.3" or "16.13.2"), falling back to the recommended major if someone has a completely unsupported major version (e.g. 15 or 17) installed, to use as the minimum version in the check. This still produces slightly misleading results in that if someone is on node 12 or 15 it won't tell them that 14 is an option, but that's probably ok since we only have 14 in there because Gutenberg Mobile is still on that version for their build (cf. #20990).

…3760) The minimum version set in package.json is like `^14.18.3 || ^16.13.2`. We weren't handling that correctly, we were comparing with a minimum "version" of "14.18.3 || ^16.13.2" rather than anything well-formed. This changes the check to extract the constraint matching the current major version of node (i.e. "14.18.3" or "16.13.2"), falling back to the recommended major if someone has a completely unsupported major version (e.g. 15 or 17) installed, to use as the minimum version in the check. This still produces slightly misleading results in that if someone is on node 12 or 15 it won't tell them that 14 is an option, but that's probably ok since we only have 14 in there because Gutenberg Mobile is still on that version for their build (cf. #20990).

This was added back in #20990 for Gutenberg Mobile. We never really tested that things still worked in Node 14. Now Gutenberg Mobile has dropped the need for the monorepo to claim Node 14 support, so let's drop it.

matticbot added the Touches WP.com Files label Sep 7, 2021

test-case-reminder bot reviewed Sep 7, 2021

View reviewed changes

dcalhoun reviewed Sep 7, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

guarani force-pushed the try/allow-node-14 branch from 8d1e707 to 6e15620 Compare September 8, 2021 01:34

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Sep 8, 2021

guarani mentioned this pull request Sep 8, 2021

Bump Jetpack submodule ref to fix node compatibility issue wordpress-mobile/gutenberg-mobile#3928

Merged

2 tasks

guarani requested a review from dcalhoun September 8, 2021 02:24

guarani mentioned this pull request Sep 8, 2021

[RNMobile] Register Jetpack Tiled Gallery for native mobile #20925

Merged

Trigger wpcom build

b30456b

anomiex approved these changes Sep 9, 2021

View reviewed changes

guarani marked this pull request as ready for review September 9, 2021 18:14

anomiex added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 9, 2021

guarani merged commit 6a6dc03 into master Sep 9, 2021

guarani deleted the try/allow-node-14 branch September 9, 2021 18:47

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 9, 2021

This was referenced Sep 21, 2021

[RNMobile] Make native Jetpack Tiled Gallery Gallery use InnerBlocks API #21135

Merged

Adding BottomSheet settings for Tiled Gallery #21028

Merged

anomiex mentioned this pull request Apr 1, 2022

tools: Fix node version check in check-development-environment.sh #23760

Merged

anomiex mentioned this pull request May 30, 2022

Use nvm to install Node js version required by Jetpack wordpress-mobile/gutenberg-mobile#4871

Merged

2 tasks

anomiex mentioned this pull request Jun 8, 2022

Remove claim of support for Node 14 #24678

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow current node LTS 14.17.6 #20990

Allow current node LTS 14.17.6 #20990

guarani commented Sep 7, 2021 •

edited

Loading

test-case-reminder bot left a comment

matticbot commented Sep 7, 2021

github-actions bot commented Sep 8, 2021 •

edited

Loading

guarani commented Sep 8, 2021

anomiex commented Sep 8, 2021 •

edited

Loading

guarani commented Sep 8, 2021

anomiex commented Sep 8, 2021

guarani commented Sep 8, 2021

guarani commented Sep 9, 2021

guarani commented Sep 9, 2021 •

edited

Loading

anomiex commented Sep 9, 2021

guarani commented Sep 9, 2021 •

edited

Loading

anomiex commented Sep 9, 2021

guarani commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

jeherve commented Sep 10, 2021

Allow current node LTS 14.17.6 #20990

Allow current node LTS 14.17.6 #20990

Conversation

guarani commented Sep 7, 2021 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

test-case-reminder bot left a comment

Choose a reason for hiding this comment

Connection

matticbot commented Sep 7, 2021

github-actions bot commented Sep 8, 2021 • edited Loading

guarani commented Sep 8, 2021

anomiex commented Sep 8, 2021 • edited Loading

guarani commented Sep 8, 2021

anomiex commented Sep 8, 2021

guarani commented Sep 8, 2021

guarani commented Sep 9, 2021

guarani commented Sep 9, 2021 • edited Loading

anomiex commented Sep 9, 2021

guarani commented Sep 9, 2021 • edited Loading

anomiex commented Sep 9, 2021

guarani commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

jeherve commented Sep 10, 2021

guarani commented Sep 7, 2021 •

edited

Loading

github-actions bot commented Sep 8, 2021 •

edited

Loading

anomiex commented Sep 8, 2021 •

edited

Loading

guarani commented Sep 9, 2021 •

edited

Loading

guarani commented Sep 9, 2021 •

edited

Loading