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

feat(CLI): add check node version middleware, rm .nvmrc, yarn engines #9728

Merged
merged 13 commits into from Dec 22, 2023

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Dec 19, 2023

Continuation of #8907. Following up on our meeting today (@Tobbe, @thedavidprice; this PR doesn't implement the configurable engines functionality yet).

This PR

  • removes the .nvmrc file from CRWA and test project fixtures

    Setup deploy commands should add node version files if they need them, preferably something nvm agnostic like Netlify's .node-version

  • removes yarn from engines in CRWA's root package.json since it doesn't do anything and just creates confusion

  • adds a node version check to build and dev

    This piece of middleware is lightweight; it doesn't involve a child process or the file system, it just checks against process.version. I also removed the babel check because it's been ages since we've made that change and original concerns against checking for the node version were about adding overhead tot he CLI. So let's remove unnecessary middleware if we're going to add more

Right now, the node version check just emits a warning. Should it error out? (I should've taken better notes.)

Build:

image

Dev:

image

Tasks

  • research deploy providers and update yarn rw setup deploy commands
  • settle on warning or error for build and dev
  • settle on the contents of the message
  • update CRWA

Deploy providers

@jtoar jtoar marked this pull request as draft December 19, 2023 20:16
@jtoar jtoar added this to the v7.0.0 milestone Dec 19, 2023
@jtoar jtoar added the release:feature This PR introduces a new feature label Dec 19, 2023
@Tobbe
Copy link
Member

Tobbe commented Dec 19, 2023

setup deploy commands should add node version files if they need them, preferably something nvm agnostic like Netlify's .node-version

I would go as far as saying "deploy target specific", not just "nvm agnostic"

For Netlify a .node-version file could be a good option. Another option is something like environment = { NODE_VERSION = "20.10.4" } in netlify.toml

Right now, the node version check just emits a warning. Should it error out? I should've taken better notes.

To me a warning makes sense for dev at least. Maybe error out on build? Or just go with a warning there as well 🤔

@jtoar jtoar marked this pull request as ready for review December 19, 2023 22:17
Copy link
Contributor

@thedavidprice thedavidprice left a comment

Choose a reason for hiding this comment

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

I like how you're handling this @jtoar! Nothing I wrote below feels like a "must do" — consider them all a part of the discussion for you and Tobbe to finalize.

Behavior for dev and build

Recommendations:

  • dev: warn
  • build: error

I didn't read the new code --> confirming you're handling cases where package.json engine isn't defined, correct?

Package.json Node Engine

"node": "=20.x" vs. "node": ">=20.x"

  • if we are only enforcing the lower bound, then I think we should use >= and let the user decide if they want to further restrict
  • if we leave it as = then the CRWA setup will allow them to use a major greater than 20 but will then error when running dev or build. If we choose to leave as =, then we should have CRWA setup update the engine value if choice is to "permit use of non-LTS supported version"

Deploy Setup

Render
Node Version is hardcoded in Template here: https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/setup/deploy/templates/render.js

  • Should we set during setup?

Netlify
I really like Tobbe's idea to use env var within Netlify toml. Makes it very clear what's going on.

Vercel
Looks like we are limited to using the package.json engine, but even that's not definitive if it is out of sync with the project dashboard setting. Also, it's much harder to understand that Vercel uses the engine. Maybe prompt during setup deploy vercel and ask user to set value, defaulting to the lower bound major in package.json engine (not sure how to handle default if no engine).

And/or maybe we just message that use needs to manage Node version in dashboard settings... ?

Doc

I'd like to see a new doc about Node.js Version Support:

  • Redwood "officially" supports the latest LTS (link to release schedule), which means CI is run against that version and packages are built to that version
  • You can choose to use latest, non-LTS versions
  • You cannot use versions prior to latest LTS version
  • How to manage via package.json engine
  • How to handle deploy providers (aka read the docs 'cause you need to manage it)
  • table showing Redwood Version and Node Version supported

We can link to the new doc when CLI gives warning or error messages and/or during deploy setup.

Thoughts?

@jtoar jtoar marked this pull request as draft December 20, 2023 06:29
@jtoar
Copy link
Contributor Author

jtoar commented Dec 20, 2023

I didn't read the new code --> confirming you're handling cases where package.json engine isn't defined, correct?

No I don't think we talked about that and I didn't think of it at all. We've shipped with it since forever, so someone would have to go in there and delete it. What were you imaging would happen in this case?

Re Render, i'd probably get rid of any trace of NODE_VERSION in the template since engines.node is enough. You're right that Vercel has more qualifications to engines.node so there should be more care there.

I know you'd like to see the doc; I didn't do it yet cause it's considerably more work.

Copy link
Contributor

thedavidprice commented Dec 20, 2023

No I don’t think we talked about that and I didn’t think of it at all. We’ve shipped with it since forever, so someone would have to go in there and delete it. What were you imaging would happen in this case?

If someone deletes it, just make sure the code doesn’t throw. :man-shrugging:

Re Render, i’d probably get rid of any trace of NODE_VERSION in the template since engines.node is enough. You’re right that Vercel has more qualifications to engines.node so there should be more care there.

Agreed about both. Might need to confirm with Render it can be removed (fwiw).

I know you’d like to see the doc; I didn’t do it yet cause it’s considerably more work.

Understood.

@jtoar jtoar marked this pull request as ready for review December 22, 2023 07:31
@jtoar jtoar merged commit 7cdf038 into main Dec 22, 2023
32 checks passed
@jtoar jtoar deleted the ds-node-version/rm-nvmrc,add-middleware-check branch December 22, 2023 18:40
Tobbe pushed a commit that referenced this pull request Dec 22, 2023
…es (#9728)

Continuation of #8907.
Following up on our meeting today (@Tobbe, @thedavidprice; this PR
doesn't implement the configurable engines functionality yet).

This PR

- removes the `.nvmrc` file from CRWA and test project fixtures

Setup deploy commands should add node version files if they need them,
preferably something nvm agnostic like Netlify's `.node-version`

- removes yarn from engines in CRWA's root package.json since it doesn't
do anything and just creates confusion

- adds a node version check to build and dev

This piece of middleware is lightweight; it doesn't involve a child
process or the file system, it just checks against `process.version`. I
also removed the babel check because it's been ages since we've made
that change and original concerns against checking for the node version
were about adding overhead tot he CLI. So let's remove unnecessary
middleware if we're going to add more

Right now, the node version check just emits a warning. Should it error
out? (I should've taken better notes.)

Build:

![image](https://github.com/redwoodjs/redwood/assets/32992335/e0d2ced6-7b52-448e-96a1-08a45aac64e0)

Dev:

![image](https://github.com/redwoodjs/redwood/assets/32992335/d78069eb-bd6c-4ac9-b936-b90f704b5c5c)

- [ ] research deploy providers and update `yarn rw setup deploy`
commands
- [x] settle on warning or error for build and dev
- [x] settle on the contents of the message
- [x] update CRWA

- [x] baremetal

  Pretty sure it's completely up to you.

- [x] coherence

Via Nixpacks, `engines.node` in the root package.json:
https://nixpacks.com/docs/providers/node#setup.

- [x] flightcontrol

Via Nixpacks, `engines.node` in the root package.json:
https://www.flightcontrol.dev/docs/getting-started/javascript/setting-node-version#using-the-packagejson-file.

- [x] netlify

Via the `.node-version` file, or via in `netlify.toml`; see
https://docs.netlify.com/configure-builds/manage-dependencies/#node-js-and-javascript,
https://docs.netlify.com/configure-builds/file-based-configuration/#sample-netlify-toml-file.

- [ ] vercel

Takes its cue from engines.node in the root package.json:
https://vercel.com/docs/functions/serverless-functions/runtimes/node-js#version-overrides-in-package.json.

- [x] render

Same as netlify more or less, but also respects `engines.node` in the
root package.json: https://docs.render.com/docs/node-version.
dac09 added a commit to dac09/redwood that referenced this pull request Dec 27, 2023
…redwood into fix/enhance-error-apollo-suspense

* 'fix/enhance-error-apollo-suspense' of github.com:dac09/redwood: (92 commits)
  chore(deps): update dependency @types/yargs to v17.0.32 (redwoodjs#9759)
  Make it easier to find useMatch docs (redwoodjs#9756)
  chore(unit tests): Use side-effect import to fix TS errors (redwoodjs#9754)
  fix(context): Refactor context (redwoodjs#9371)
  docs: Replaced deprecated <Set private> with PrivateSet within router.md (redwoodjs#9749)
  add TS support for storybook preview tsx config extension (redwoodjs#9309)
  fix(studio): Fix windows path issues (redwoodjs#9752)
  redwoodjs#9620: Update studio to support variable components (Mailer) (redwoodjs#9639)
  chore(tasks): Add comparison view to nmHoisting visualisation (redwoodjs#9751)
  chore(cli): make fs modules used in the CLI consistent (redwoodjs#9746)
  chore(tooling): Make sure console boxen print on a new line
  chore(CI): fix publish release candidate
  feat(CLI): add check node version middleware, rm `.nvmrc`, yarn engines (redwoodjs#9728)
  docs: added some clarification on serverless functions getting executed in a non-serverless environment (redwoodjs#9742)
  Fix sshExec() errors not displaying (redwoodjs#9743)
  chore(tooling): Add missing word in release tooling output
  Update Metadata docs (redwoodjs#9744)
  chore(CI): update test project fixture and CRWA for deploy target CI repo (redwoodjs#9730)
  chore(tooling): add script for getting nested dependency data (redwoodjs#9734)
  Trusted Documents docs: Proofreading corrections (redwoodjs#9737)
  ...
dac09 added a commit to dac09/redwood that referenced this pull request Dec 27, 2023
…ath-aliases

* 'main' of github.com:redwoodjs/redwood: (92 commits)
  chore(deps): update dependency @types/yargs to v17.0.32 (redwoodjs#9759)
  Make it easier to find useMatch docs (redwoodjs#9756)
  chore(unit tests): Use side-effect import to fix TS errors (redwoodjs#9754)
  fix(context): Refactor context (redwoodjs#9371)
  docs: Replaced deprecated <Set private> with PrivateSet within router.md (redwoodjs#9749)
  add TS support for storybook preview tsx config extension (redwoodjs#9309)
  fix(studio): Fix windows path issues (redwoodjs#9752)
  redwoodjs#9620: Update studio to support variable components (Mailer) (redwoodjs#9639)
  chore(tasks): Add comparison view to nmHoisting visualisation (redwoodjs#9751)
  chore(cli): make fs modules used in the CLI consistent (redwoodjs#9746)
  chore(tooling): Make sure console boxen print on a new line
  chore(CI): fix publish release candidate
  feat(CLI): add check node version middleware, rm `.nvmrc`, yarn engines (redwoodjs#9728)
  docs: added some clarification on serverless functions getting executed in a non-serverless environment (redwoodjs#9742)
  Fix sshExec() errors not displaying (redwoodjs#9743)
  chore(tooling): Add missing word in release tooling output
  Update Metadata docs (redwoodjs#9744)
  chore(CI): update test project fixture and CRWA for deploy target CI repo (redwoodjs#9730)
  chore(tooling): add script for getting nested dependency data (redwoodjs#9734)
  Trusted Documents docs: Proofreading corrections (redwoodjs#9737)
  ...
jtoar added a commit that referenced this pull request Jan 6, 2024
Follow up to #9783. This PR
converts the `create-redwood-app` package to ESM and bundles all its
dependencies. I started with `create-redwood-app` because the
requirements for making it ESM were relatively trivial compared to the
other packages since it just needs to be run by `yarn create`, and yarn
create just runs a bin.

For `create-redwood-app` I'm just using esbuild. Why not use tsup?
1. We're just distributing a bin. We're not distributing a dual-module
package with types
2. tsup hasn't been committed to in over a month and a half, whereas
esbuild releases often and is committed to more-or-less daily. That
doesn't automatically disqualify it (OSS is hard), but makes me wary.
I'll consider it for packages that distribute more than a bin

More on bundling. Bundling this package has benefits, namely decreasing
the install time—yarn doesn't have to fetch its dependencies, there are
none. But I'm mostly doing it as an exercise because we need to do it
more. For background on the shims (`jsBanner`) see
evanw/esbuild#1921. It's nothing bespoke and
it's what tsup[^tsup] and Vite[^vite] would've done anyway.

Other notes:

- Updates the package's `README.md`; this could be updated more but I
didn't want to spend too much time on it

- Adds e2e tests for the node version check

Two new e2e tests make sure we're checking node version correctly. I
can't use nvm since 1. it's not easily scriptable (it's a shell built-in
or something) and 2. it doesn't seem like we all use it, so I just added
these tests to CI and use the GitHub action to change the node version

- Fixes a bug I introduced in
#9728

The node version check would throw if it didn't pass because
`engines.yarn` was removed. This wasn't released

- Converted files to just `.js` since Node recognized them as ESM from
`type` in `package.json`

- Reordered `yarn create-redwood-app`'s options in help; I tried to put
the ones that were more likely to be used first

- Removed the header from `--help` and `--version`

[^tsup]:
https://github.com/egoist/tsup/blob/8c26e63c92711d60c05aedd3cdc358530ba266c5/assets/esm_shims.js
[^vite]:
https://github.com/vitejs/vite/blob/8de7bd2b68db27b83d9484cc8d4e26436615168e/packages/vite/rollup.config.ts#L288-L295
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants