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.

Sign up for GitHub

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

Jump to bottom

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? #24891

Closed
0xdevalias opened this issue Jul 12, 2022 · 3 comments
Closed

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? #24891

0xdevalias opened this issue Jul 12, 2022 · 3 comments

Comments

@0xdevalias
Copy link
Contributor

0xdevalias commented Jul 12, 2022 •
edited
Loading

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:

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

Looking at the git history for appveyor.yml:

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

It was updated to use NodeJS 10.x in:

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:

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

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:

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:

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

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

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

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:

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:
This was referenced Jul 12, 2022
Closed
Closed
@0xdevalias
Copy link
Contributor Author

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?

@0xdevalias 0xdevalias mentioned this issue Jul 12, 2022
Closed
@0xdevalias
Copy link
Contributor Author

0xdevalias commented Jul 13, 2022 •
edited
Loading

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-)

This was referenced Jul 13, 2022
Closed
Merged
sebmarkbage added a commit that referenced this issue Feb 20, 2023
@0xdevalias @sebmarkbage
Update Rollup and related plugins to their most recent versions (#24916)
6b6d061 
Update Rollup and related plugins to their most recent versions +
resolve any breaking changes/deprecations/etc along the way. I made each
change piece by piece, so the commit history tells a pretty good story
of what was changed where/how/why.

fixes #24894

For the full deepdive/context, see:

- #24894

The inspiration for this came from @jasonwilliams 's PR for attempting
to add sourcemap output support to React's builds:

- #20186
  - #21946

But I figured that it would be useful to minimise the scope of changes
in that PR, and to modernise the build tooling along the way.

If any of these updates rely on a node version later than `10.x`, then
the following PR may have to land first, otherwise things might break on
AppVeyor:

- #24891
  - #24892

Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>
github-actions bot pushed a commit that referenced this issue Feb 20, 2023
@sebmarkbage
Update Rollup and related plugins to their most recent versions (#24916)
7b76862 
Update Rollup and related plugins to their most recent versions +
resolve any breaking changes/deprecations/etc along the way. I made each
change piece by piece, so the commit history tells a pretty good story
of what was changed where/how/why.

fixes #24894

For the full deepdive/context, see:

- #24894

The inspiration for this came from @jasonwilliams 's PR for attempting
to add sourcemap output support to React's builds:

- #20186
  - #21946

But I figured that it would be useful to minimise the scope of changes
in that PR, and to modernise the build tooling along the way.

If any of these updates rely on a node version later than `10.x`, then
the following PR may have to land first, otherwise things might break on
AppVeyor:

- #24891
  - #24892

Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>

DiffTrain build for [6b6d061](6b6d061)
[View git log for this commit](https://github.com/facebook/react/commits/6b6d0617eff48860c5b4e3e79c74cbd3312cf45a)
@0xdevalias
Copy link
Contributor Author

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 #24892 (comment)

jerrydev0927 added a commit to jerrydev0927/react that referenced this issue Jan 5, 2024
@jerrydev0927
Update Rollup and related plugins to their most recent versions (#24916)
ca34f21 
Update Rollup and related plugins to their most recent versions +
resolve any breaking changes/deprecations/etc along the way. I made each
change piece by piece, so the commit history tells a pretty good story
of what was changed where/how/why.

fixes facebook/react#24894

For the full deepdive/context, see:

- facebook/react#24894

The inspiration for this came from @jasonwilliams 's PR for attempting
to add sourcemap output support to React's builds:

- facebook/react#20186
  - facebook/react#21946

But I figured that it would be useful to minimise the scope of changes
in that PR, and to modernise the build tooling along the way.

If any of these updates rely on a node version later than `10.x`, then
the following PR may have to land first, otherwise things might break on
AppVeyor:

- facebook/react#24891
  - facebook/react#24892

Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>

DiffTrain build for [6b6d0617eff48860c5b4e3e79c74cbd3312cf45a](facebook/react@6b6d061)
[View git log for this commit](https://github.com/facebook/react/commits/6b6d0617eff48860c5b4e3e79c74cbd3312cf45a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

bump AppVeyor Node version from '10.x' to '14.x' (current Maintenance LTS version) 0xdevalias/react
1 participant
@0xdevalias