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

chore: drop Node.js 12 and modernize the toolchain #13705

Merged
merged 7 commits into from Jun 9, 2022
Merged

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented Jun 8, 2022

  • Stop testing on Node.js 12, start testing on Node.js 18
    (i.e., test on Node.js 14, 16, and 18).

Before the changes:

Workflow Node.js Version
Buildkite (Tests): 14
Buildkite (Release): 12
GitHub Actions (Tests): 12, 16
GitHub Actions (Other Jobs): 12

After the changes:

Workflow Node.js Version
Buildkite (Tests): 14
Buildkite (Release): 14
GitHub Actions (Tests): 16, 18
GitHub Actions (Other Jobs): 16
  • Update the example commands in CONTRIBUTING.md:

    • Update the nvm installation command.

    • Update the Node.js installation command to install Node.js 16 since
      the text recommends using Active LTS and Node.js 14 is not Active
      LTS anymore (it's already Maintenance LTS and on track to becoming
      EOL in 2023).

  • Update the VSCode Dev Container to use the current Active LTS, update
    the comments there.

  • Prepare to un-pin ts-node in a follow-up PR.
    Update ts-node in packages/{client,debug} from 28.0.3 to 28.0.4
    because ts-node@28.0.3 has a peer dependency on @types/jest@27
    and new pnpm now errors on incorrect peer dependency instead of just
    printing a warning.

Ref: https://github.com/prisma/client-planning/issues/19

@aqrln aqrln requested a review from a team June 8, 2022 15:29
@aqrln aqrln requested review from Jolg42 and jkomyno as code owners June 8, 2022 15:29
@aqrln aqrln requested review from millsp and removed request for a team June 8, 2022 15:29
@aqrln aqrln added this to the 4.0.x milestone Jun 8, 2022
@Jolg42
Copy link
Member

Jolg42 commented Jun 8, 2022

@aqrln this is from Buildkite

[2022-06-08T15:30:52Z] /app/scripts/setup.ts
[2022-06-08T15:30:52Z]   150:1  error  Async function 'getPrismaCommitFromPackageJsonViaNpmRegistry' has no 'await' expression  @typescript-eslint/require-await

@SevInf
Copy link
Contributor

SevInf commented Jun 9, 2022

Should we somehow enforce minimum pnpm major version, maybe in scripts/only-allow-pnpm? Right now pnpm 6 and 7 produce different versions of lockfiles, mutually warn about it and update them.

Copy link
Member

@Jolg42 Jolg42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SevInf Good point we should update the engines in the root package.json, currently

  "engines": {
    "node": ">=12.6",
    "pnpm": ">=6.14.1"
  },

Copy link
Member

@millsp millsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should update this if package some managers just emit a warning on bad node engine versions:
https://github.dev/prisma/prisma/blob/0d6028db17995bfdc6111f23e065bd1829556450/packages/cli/scripts/preinstall.js#L45-L48

@aqrln
Copy link
Member Author

aqrln commented Jun 9, 2022

I get the same linter errors locally as on Buildkite. What's weird is why they don't happen on GitHub Actions, and didn't happen on Buildkite when running ESLint with Node.js 12.

@Jolg42
Copy link
Member

Jolg42 commented Jun 9, 2022

@aqrln probably depends on Node.js version, should be easy to remove async from the function definition I think

@aqrln
Copy link
Member Author

aqrln commented Jun 9, 2022

probably depends on Node.js version

That was my first thought to, but it would be weird for the rule to work on Node.js 14 (Buildkite) and Node.js 18 (locally) but not on Node.js 16 (GH Actions). But it's easy to check if that's the case by switching the version locally.

@aqrln
Copy link
Member Author

aqrln commented Jun 9, 2022

Same errors with Node.js 16 and even Node.js 12 locally :/

Something is wrong with our CI workflows for linting.

@Jolg42
Copy link
Member

Jolg42 commented Jun 9, 2022

@aqrln but we run the same command pnpm run lint for both so let's ignore that for now I guess

@aqrln
Copy link
Member Author

aqrln commented Jun 9, 2022

@Jolg42 yeah it's definitely out of scope here, but something to investigate

@Jolg42
Copy link
Member

Jolg42 commented Jun 9, 2022

Do you know if someone is already planning to update the docs like https://www.prisma.io/docs/reference/system-requirements?

Maybe open a PR there

@Jolg42
Copy link
Member

Jolg42 commented Jun 9, 2022

@aqrln could be that the problems in test failures are from pnpm v7, maybe separate this into another PR

aqrln added 6 commits June 9, 2022 14:22
- Stop testing on Node.js 12, start testing on Node.js 18
  (i.e., test on Node.js 14, 16, and 18).

*Before the changes:*

| Workflow                     | Node.js Version    |
| ---------------------------- | ------------------ |
| Buildkite (Tests):           | 14                 |
| Buildkite (Release):         | 12                 |
| GitHub Actions (Tests):      | 12, 16             |
| GitHub Actions (Other Jobs): | 12                 |

*After the changes:*

| Workflow                     | Node.js Version    |
| ---------------------------- | ------------------ |
| Buildkite (Tests):           | 14                 |
| Buildkite (Release):         | 14                 |
| GitHub Actions (Tests):      | 16, 18             |
| GitHub Actions (Other Jobs): | 16                 |

- Update the example commands in CONTRIBUTING.md:

  - Update the nvm installation command.

  - Update the Node.js installation command to install Node.js 16 since
    the text recommends using Active LTS and Node.js 14 is not Active
    LTS Anymore (it's already Maintenance LTS and on track to becoming
    EOL in 2023).

- Update the VSCode Dev Container to use the current Active LTS, update
  the comments there.

- Un-pin pnpm version and use latest, update the lockfile.

- Update ts-node in `packages/{client,debug}` from 28.0.3 to 28.0.4
  because `ts-node@28.0.3` has a peer dependency on `@types/jest@27`
  and new pnpm now errors on incorrect peer dependency instead of just
  printing a warning.
@aqrln aqrln merged commit da15e1f into main Jun 9, 2022
@aqrln aqrln deleted the prisma4/drop-nodejs-12 branch June 9, 2022 16:22
@Jolg42 Jolg42 mentioned this pull request Jun 10, 2022
1 task
SevInf pushed a commit that referenced this pull request Jun 10, 2022
- Stop testing on Node.js 12, start testing on Node.js 18
  (i.e., test on Node.js 14, 16, and 18).

*Before the changes:*

| Workflow                     | Node.js Version    |
| ---------------------------- | ------------------ |
| Buildkite (Tests):           | 14                 |
| Buildkite (Release):         | 12                 |
| GitHub Actions (Tests):      | 12, 16             |
| GitHub Actions (Other Jobs): | 12                 |

*After the changes:*

| Workflow                     | Node.js Version    |
| ---------------------------- | ------------------ |
| Buildkite (Tests):           | 14                 |
| Buildkite (Release):         | 14                 |
| GitHub Actions (Tests):      | 16, 18             |
| GitHub Actions (Other Jobs): | 16                 |

- Update the example commands in CONTRIBUTING.md:

  - Update the nvm installation command.

  - Update the Node.js installation command to install Node.js 16 since
    the text recommends using Active LTS and Node.js 14 is not Active
    LTS Anymore (it's already Maintenance LTS and on track to becoming
    EOL in 2023).

- Update the VSCode Dev Container to use the current Active LTS, update
  the comments there.

- Prepare to un-pin ts-node in a follow-up PR.
  Update ts-node in `packages/{client,debug}` from 28.0.3 to 28.0.4
  because `ts-node@28.0.3` has a peer dependency on `@types/jest@27`
  and new pnpm now errors on incorrect peer dependency instead of just
  printing a warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants