-
Notifications
You must be signed in to change notification settings - Fork 47k
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
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
Comments
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? |
Looking at the
Looking at
Presumably this would mean that on AppVeyor, when using a As a contrived example, I locally edited
(I also noticed this doesn't seem to set any version for |
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 <[email protected]>
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 <[email protected]> DiffTrain build for [6b6d061](6b6d061) [View git log for this commit](https://github.com/facebook/react/commits/6b6d0617eff48860c5b4e3e79c74cbd3312cf45a)
Based on the following comment on the associated PR I opened, I believe this issue can now be closed as well:
|
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 <[email protected]> DiffTrain build for [6b6d0617eff48860c5b4e3e79c74cbd3312cf45a](facebook/react@6b6d061) [View git log for this commit](https://github.com/facebook/react/commits/6b6d0617eff48860c5b4e3e79c74cbd3312cf45a)
tl;dr
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
(nowmain
) 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 frommaster
: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 thecimg/openjdk:17.0.0-node
docker container:17.0.0-node
uses: node 14.17.6, yarn 1.22.5Looking at
.codesandbox/ci.json
, it appears to use node14.x
.Looking at
.nvmrc
, it appears to use node4.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:
8.x
is default Node.js installed on build workers16.8.0
(x86 and x64) - useCurrent
alias for latest16.x
release15.0.0
- 15.14.0 (x86 and x64)14.17.6
(x86 and x64) - default on VS 2019 image. UseLTS
alias for latest14.x
releaseSo 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 the14.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 supportss.replace
, but this version requires Node12.x
or higher, which currently would presumably break on AppVeyor as it uses node10.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:
11.x
and12.x
alongside the existing10.x
in the AppVeyor testing matrixThe text was updated successfully, but these errors were encountered: