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.

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

bump AppVeyor Node version from '10.x' to '14.x' (current Maintenance LTS version) #24892

Closed

Conversation

0xdevalias
Copy link
Contributor

@0xdevalias 0xdevalias commented Jul 12, 2022

Summary

fixes #24891

For the full deepdive/context, see:

The below is an excerpt from the above that is specifically relevant to this PR:

If 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.

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:

How did you test this change?

I haven't.. but hopefully it will run automagically (or someone else can trigger it to run since AppVeyor is only set up to run on the main branch)

@sizebot
Copy link

sizebot commented Jul 12, 2022

Comparing: e225fa4...15a04f3

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 133.13 kB 133.13 kB = 42.76 kB 42.76 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 138.41 kB 138.41 kB = 44.34 kB 44.34 kB
facebook-www/ReactDOM-prod.classic.js = 465.92 kB 465.92 kB = 84.38 kB 84.38 kB
facebook-www/ReactDOM-prod.modern.js = 451.16 kB 451.16 kB = 82.14 kB 82.14 kB
facebook-www/ReactDOMForked-prod.classic.js = 465.92 kB 465.92 kB = 84.38 kB 84.38 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 15a04f3

kassens added a commit to kassens/react that referenced this pull request Sep 30, 2022
I noticed this file as facebook#24892 is updating the NodeJS version in it.

I don't think this is used for anything as the config just runs a handful
of yarn commands and I assume that's all covered by CircleCI.
kassens added a commit to kassens/react that referenced this pull request Sep 30, 2022
I noticed this file as facebook#24892 is updating the NodeJS version in it.

I don't think this is used for anything as the config just runs a handful
of yarn commands and I assume that's all covered by CircleCI.
kassens added a commit that referenced this pull request Sep 30, 2022
I noticed this file as #24892 is updating the NodeJS version in it.

I don't think this is used for anything as the config just runs a handful
of yarn commands and I assume that's all covered by CircleCI.
@kassens
Copy link
Member

kassens commented Oct 3, 2022

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

@kassens kassens closed this Oct 3, 2022
@0xdevalias 0xdevalias deleted the 0xdevalias/bump-appveyor-node branch October 4, 2022 21:32
sebmarkbage added a commit that referenced this pull request Feb 20, 2023
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]>
github-actions bot pushed a commit that referenced this pull request Feb 20, 2023
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)
jerrydev0927 added a commit to jerrydev0927/react that referenced this pull request Jan 5, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants