Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Run on latest 12.x #2065

Closed
wants to merge 1 commit into from
Closed

Run on latest 12.x #2065

wants to merge 1 commit into from

Conversation

atesgoral
Copy link
Contributor

Description

Tweak CI matrix to run on 12.x instead of 12.14 (Don't know the historical context of being on 12.14)

Type of change

Just a CI environment tweak. No change to any code.

@atesgoral atesgoral requested a review from a team November 2, 2021 14:29
@michenly
Copy link
Contributor

michenly commented Nov 2, 2021

Wondering if this should sync to the v12 version or not

nodeTargets: 'node 12.14.0',
(maybe this is why we test for 12.14.0, because we build with it)

@BPScott
Copy link
Member

BPScott commented Nov 2, 2021

Don't know the historical context of being on 12.14

We currently say 12.14.0 is our minimum supported version. With '12.x' CI will run on the latest v12 version (12.22.7 according to this)

Consider a world where node added some APIs between the minimum stated supported version (12.14) and the minimum version we run CI against (12.22). It we were to use those APIs then things would blow up for consumers using 12.14 but we wouldn't know because we never run our tests against the minimum version that we claim support.

Strictly speaking, If we were to require a specific minor version in v14/v16 then the same would go for v14/v16 releases too (e.g. in the loom repo, packages have an engines field of "^12.22.0 || ^14.17.0 || >=16.0.0" so we should test on v14.17.0, rather than 14.x). However in quilt's case this doesn't apply for as the quilt packages have engines fields of >=12.14.0. So for quilt I think it's fine for the test matrix to be ['12.14', '14.x', '16.x'].

I think it is useful for CI to run against the minimum versions that we claim to support rather the latest release in a major version so that we can ensure that we don't accidentally use a feature that is not present in old-but-supported node versions and thus break consumers that are still using that version. This is the same rational for configuring dev.yml to use the oldest supported node version (in quilt's case 12.14) rather than the latest version - except now can no longer do that thanks to local dev requiring node 16 thanks to M1 chips.

Yeah but what does that mean

  • I think it's fine to close this PR without merge as I think testing the lowest supported version in each major branch is acceptable - and for quilt that is v12.14, and quilt currently has no opinions on required minors for v14/v16.
  • For packages that require specific versions of node LTS versions (e.g. loom) we should test the lowest supported version of each LTS version - so for loom with its engines of "^12.22.0 || ^14.17.0 || >=16.0.0" the test matrix should be ['12.22', '14.17', '16.0']

@atesgoral atesgoral closed this Nov 5, 2021
@atesgoral atesgoral deleted the use-latest-12 branch November 5, 2021 12:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants