react icon indicating copy to clipboard operation
react copied to clipboard

Published 20 hours ago • verified publisher icon facebook

Is AppVeyor still used to test against Windows, can it be moved to CircleCI, and/or can we update it to use a modern/supported version of Node?

Open 0xdevalias opened this issue 3 years ago • 2 comments
trafficstars

tl;dr

  • Is AppVeyor still used to test against Windows?
  • Can it be moved to CircleCI?
  • If not, can we update to use a modern version of node for it?

DeepDive into context and specifics

Currently the main CI testing seems to be done on CircleCI:

  • https://github.com/facebook/react/blob/main/.circleci/config.yml

Yet there is also an AppVeyor config that appears to be used for testing against Windows:

  • https://github.com/facebook/react/blob/main/appveyor.yml

Looking at the git history for appveyor.yml:

  • https://github.com/facebook/react/commits/main/appveyor.yml

AppVeyor appears to have been added to run against the master (now main) branch in:

  • https://github.com/facebook/react/issues/11595
    • https://github.com/facebook/react/pull/11605

It was updated to use NodeJS 10.x in:

  • https://github.com/facebook/react/pull/13241

There was an attempt to move the Windows build from AppVeyor to CircleCI in #17984, but that was reverted in #18302 due to it slowing down the CI time:

  • https://github.com/facebook/react/pull/17984

    • @wittgenst: Migrate the MS Windows continuous integration configuration from AppVeyor to CircleCI

    • Docs
      • https://circleci.com/docs/using-windows
      • https://circleci.com/developer/orbs/orb/circleci/windows
      • https://circleci.com/docs/hello-world-windows
  • https://github.com/facebook/react/pull/18302

    • @acdlite: I'm reverting this because it slowed our CI times from ~6 minutes to ~23 minutes. That's because it runs the build command without concurrency.

It was updated to run on the main branch when it was renamed from master:

  • https://github.com/facebook/react/issues/21767
    • https://github.com/facebook/react/pull/21768

I stumbled across a few links to a publicly accessible AppVeyor project, but looking at the build output from that, it doesn't look like it's been built against for ~2 years:

  • https://ci.appveyor.com/project/Facebook/react
  • https://ci.appveyor.com/project/Facebook/react/history

I realise that there might be a different/non-public AppVeyor project that it is being built against now though.

If it's AppVeyor is still in use, and it's still not possible to migrate the Windows builds to CircleCI, it would be good to upgrade to a modern version of node:

  • https://nodejs.org/en/about/releases/
    • node 14.x is the current Maintenance LTS
    • node 16.x is the current Active LTS

Looking at .circleci/config.yml, the jobs seem to run on the cimg/openjdk:17.0.0-node docker container:

  • https://circleci.com/developer/images/image/cimg/openjdk
    • 17.0.0-node uses: node 14.17.6, yarn 1.22.5

Looking at .codesandbox/ci.json, it appears to use node 14.x.

  • https://github.com/facebook/react/commits/main/.codesandbox/ci.json
    • https://github.com/facebook/react/pull/22401
    • https://github.com/facebook/react/pull/21364
    • https://github.com/facebook/react/pull/17175

Looking at .nvmrc, it appears to use node 4.17.6:

  • https://github.com/facebook/react/commits/main/.nvmrc
    • https://github.com/facebook/react/pull/22401
    • https://github.com/facebook/react/pull/21430
    • https://github.com/facebook/react/pull/18623
    • https://github.com/facebook/react/pull/16610

Looking at package.json, devEngines is set to "node": "^12.17.0 || 13.x || 14.x || 15.x || 16.x || 17.x"

AppVeyor's Windows images include the following versions of node by default:

  • https://www.appveyor.com/docs/windows-images-software/#node-js
    • 8.x is default Node.js installed on build workers
    • Node.js 16.8.0 (x86 and x64) - use Current alias for latest 16.x release
    • Node.js 15.0.0 - 15.14.0 (x86 and x64)
    • Node.js 14.17.6 (x86 and x64) - default on VS 2019 image. Use LTS alias for latest 14.x release
    • etc

So I think that either we should upgrade both AppVeyor and CircleCI to use a 16.x version of node, or at the very least, should upgrade AppVeyor to use the 14.x version (i'm unsure if there is any official list of node versions that React is intended to support being built on, but if so, then should probably test against all of those versions)

While this may not be too important currently (as presumably all of the build tools work at the moment), it will likely become more of an issue as time goes on, and the build tooling is upgraded to versions that will require newer versions of the node runtime:

eg. An improvement that would allow generating source maps for React would ideally be able to use a version of the magic-string library (which is used by a number of the Rollup plugins) that supports s.replace, but this version requires Node 12.x or higher, which currently would presumably break on AppVeyor as it uses node 10.x:

  • https://github.com/facebook/react/issues/20186
    • https://github.com/facebook/react/pull/21946#discussion_r918677066
  • https://github.com/Rich-Harris/magic-string/blob/master/CHANGELOG.md#0260-2022-03-03
  • https://github.com/rollup/plugins/search?q=magic-string&type=code

I found some previous PR's that attempted to make some of these sorts of changes, but they never seemed to land for various reasons:

  • https://github.com/facebook/react/pull/17197
    • This PR wanted to add node 11.x and 12.x alongside the existing 10.x in the AppVeyor testing matrix

    • @Saibamen Test with Node 11 and 12

    • @threepointone It usually works with later versions if it passes older versions, so I must ask: what specifically should be tested with newer versions? Is there something that breaks currently? Have you seen issues with other projects that could have been prevented by testing on newer versions? Happy to approve this if there’s a concrete reason.

    • @acdlite Can we replace AppVeyor with a CircleCI workflow instead? My understanding is that we only use AppVeyor because CircleCI used to not support Windows. But now it does: https://circleci.com/blog/windows-general-availability-announcement/

  • https://github.com/facebook/react/pull/24301
    • This PR attempts to upgrade to yarn 3.x, enables corepack, updates AppVeyor's node version to 16.x, etc

    • @tiziodcaio I upgraded to yarn v3.2.0, which would not work anymore on nodejs 10

    • @kachkaev Upgrading to Yarn 3 is blocked by the lack of Dependabot support: https://github.com/dependabot/dependabot-core/issues/1297

0xdevalias avatar Jul 12 '22 09:07 0xdevalias

Given there seem to be so many different places that define various Node versions across the codebase, I wonder if it would make sense to add some kind of automated PR checker using Danger JS or similar to ensure these fields are kept in sync with one another?

  • https://danger.systems/js/
  • https://github.com/danger/danger-js
  • https://github.com/facebook/react/blob/main/dangerfile.js

0xdevalias avatar Jul 12 '22 10:07 0xdevalias

Looking at the postinstall script in the main package.json, we can see the following:

"postinstall": "node node_modules/fbjs-scripts/node/check-dev-engines.js package.json && node ./scripts/flow/createFlowConfigs.js && node ./scripts/yarn/downloadReactIsForPrettyFormat.js",

Looking at node_modules/fbjs-scripts/node/check-dev-engines.js it checks whether the current node version matches the semver ranges defined in package.json's devEngines.node:

"devEngines": {
    "node": "^12.17.0 || 13.x || 14.x || 15.x || 16.x || 17.x"
  },

Presumably this would mean that on AppVeyor, when using a 10.x version of node, this script should already be failing during the yarn install step, which makes me wonder more if AppVeyor is still being run/used.

As a contrived example, I locally edited devEngines.node to only have 20.x (while I am using a 16.15.1 version of node), then ran yarn install; and it threw the following error during postinstall:

$ node node_modules/fbjs-scripts/node/check-dev-engines.js package.json && node ./scripts/flow/createFlowConfigs.js && node ./scripts/yarn/downloadReactIsForPrettyFormat.js
node:assert:399
    throw err;
    ^

AssertionError [ERR_ASSERTION]: Current node version is not supported for development, expected "16.15.1" to satisfy "20.x".
    at Object.<anonymous> (/Users/devalias/dev/facebook/react/node_modules/fbjs-scripts/node/check-dev-engines.js:46:3)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}
error Command failed with exit code 1.

(I also noticed this doesn't seem to set any version for yarn, which while it isn't supported by check-dev-engines.js, it seemingly is in yarn itself: https://classic.yarnpkg.com/en/docs/package-json#engines-)

0xdevalias avatar Jul 13 '22 02:07 0xdevalias

Based on the following comment on the associated PR I opened, I believe this issue can now be closed as well:

The unused config was now deleted. Thanks for drawing attention at it!

Originally posted by @kassens in https://github.com/facebook/react/issues/24892#issuecomment-1265459348

0xdevalias avatar Mar 29 '23 01:03 0xdevalias