feat: Defaulting nodeVersion to system #18732

mjhenkes · 2021-11-01T18:17:46Z

Closes Have default nodeVersion be set to system instead of bundled #18684

User facing changelog

Breaking:
The nodeVersion configuration option now defaults to system. The behavior of the system option has changed to always use the node binary/version of the cli that launched the cypress app. If the cli was not used to launch the app, cypress will use the node version bundled with electron.

Suggestion:
Breaking:

The nodeVersion configuration option now defaults to system, which uses your node version instead of the built in one in Cypress. We don't anticipate this causing any issues, but if you'd like to revert to the old behavior, set nodeVersion back to 'bundled'.
Additionally, the meaning of system has changed - previous Cypress would search for the node version that was default to your system, and now it uses whatever node version was used to launch cypress from the CLI.

Additional details

How has the user experience changed?

There are now two deprecation messages.

When "nodeVersion": "bundled" is set:

When "nodeVersion": "system" is set:

There is no deprecation notice when nodeVersion is unset.

We also now display the node version by default when running tests run mode.

Before:

After:

PR Tasks

Have tests been added/updated?
Has the original issue or this PR been tagged with a release in ZenHub?
Has a PR for user-facing changes been opened in cypress-documentation? cypress-docs PR
Have API changes been updated in the type definitions?

cypress-bot · 2021-11-01T18:17:49Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2021-11-01T18:33:04Z

Test summary

4214 • 0 • 50 • 2 • 0

Run details


Project	cypress
Status	Passed
Commit	`f8fd333`
Started	Nov 4, 2021 4:57 PM
Ended	Nov 4, 2021 5:07 PM
Duration	09:52 💡
OS	Linux Debian - 10.9
Browser	Chrome beta 96

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

brian-mann · 2021-11-02T00:16:27Z

cli/lib/exec/spawn.js

@@ -81,7 +81,7 @@ module.exports = {
      args = [args]
    }

-    args = [...args, '--cwd', process.cwd()]
+    args = [...args, '--cwd', process.cwd(), '--cliNodePath', process.execPath, '--cliNodeVersion', process.versions.node]


I would call these args --userNodePath and --userNodeVersion instead of CLI, since CLI is an implementation detail, but user is the actual descriptor.

emilyrohrbough · 2021-11-02T13:36:14Z

system-tests/__snapshots__/system_node_spec.js

@@ -5,11 +5,10 @@ exports['e2e system node uses system node when launching plugins file 1'] = `
  (Run Starting)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
-  │ Cypress:        1.2.3                                                                          │
-  │ Browser:        FooBrowser 88                                                                  │
-  │ Node Version:   vX (/foo/bar/node)                                                             │


It still might be useful to log this message for anyone that might spawn this programmatically or provide an override version via the CLI.

The presence of the node version message is based on whether resolvedNodePath is undefined or not. In this case since the test is not initiated via the cypress cli, there isn't a userNodePath or version defined so the test falls back to the bundled node version. So this feature isn't removed it's just not being triggered in this test.

https://github.com/cypress-io/cypress/blob/develop/packages/server/lib/modes/run.js#L151

I figured out a better way to fix this test so the snapshot should be the same now.

emilyrohrbough

If the cli was not used to launch the app, cypress will use the node version bundled with electron.

Any way for user to set the version from the config.json if they launch programmatically?

brian-mann · 2021-11-02T13:50:53Z

We may want to consider removing nodeVersion from cypress.json entirely.

The GUI will already tell you which node version you're using, and I don't really see a use case anymore for forcing it to be bundled.

mjhenkes · 2021-11-02T14:00:30Z

@brian-mann it makes sense to remove nodeVersion. I wasn't sure if there was a case where using the user node version may be a problem and we should give the user a way to fall back to the bundled version.

mjhenkes · 2021-11-02T14:08:53Z

@emilyrohrbough

Any way for user to set the version from the config.json if they launch programmatically?

no, there is not a way to set an exact node version in the config. I think we're trying to avoid that?

emilyrohrbough

Looks good. Just need the docs PR.

jennifer-shehane · 2021-11-02T15:31:13Z

@brian-mann I agree that I don't see much value in offering the nodeVersion config value in the future. I would like to deprecate the nodeVersion config through the next major version and remove it after that.

I think that deprecating will help de-risk the initial change to system node. People will have a fallback in case there is some edge case we may have missed (with windows file system or something for example). They can quickly switch to bundle instead of us having to rush out a patch release.

…nodeVersion`

mjhenkes · 2021-11-02T17:46:10Z

packages/server/lib/config.ts

@@ -306,7 +307,6 @@ export function mergeDefaults (config: Record<string, any> = {}, options: Record
  return setSupportFileAndFolder(config)
  .then(setPluginsFile)
  .then(setScaffoldPaths)
-  .then(_.partialRight(setNodeBinary, options.onWarning))


no more promises here so we can just call the function

mjhenkes · 2021-11-02T17:47:04Z

packages/server/lib/config_options.ts

@@ -151,8 +151,7 @@ export const options = [
    isInternal: true,
  }, {
    name: 'nodeVersion',
-    defaultValue: 'default',


The default value is removed so that the depreciation warning will only fire when the 'nodeVersion' config is present.

mjhenkes · 2021-11-03T21:31:40Z

packages/server/lib/config.ts

@@ -65,8 +64,13 @@ const convertRelativeToAbsolutePaths = (projectRoot, obj, defaults = {}) => {
 }

 const validateNoBreakingConfig = (cfg) => {
-  return _.each(breakingOptions, ({ name, errorKey, newName, isWarning }) => {
-    if (_.has(cfg, name)) {
+  breakingOptions.forEach(({ name, errorKey, newName, isWarning, value }) => {


Updated this to allow me to flex the warning message on the value of the config option

brian-mann · 2021-11-04T01:19:03Z

packages/server/lib/errors.js

@@ -1010,6 +1010,16 @@ const getMsgByType = function (type, arg1 = {}, arg2, arg3) {
        ${arg1 ? 'Try installing Node.js 64-bit and reinstalling Cypress to use the 64-bit build.'
        : 'Consider upgrading to a 64-bit OS to continue using Cypress.'}
        `
+    case 'NODE_VERSION_DEPRECATION_SYSTEM':
+      return stripIndent`\
+      Deprecation Warning: ${chalk.yellow(`\`nodeVersion\``)} is currently set to ${chalk.yellow(`\`system\``)} in the ${chalk.yellow(`\`cypress.json\``)} configuration file. As of Cypress v9 the default behavior of ${chalk.yellow(`\`nodeVersion\``)} has changed to always use the version of Node used to start cypress via the cli.


nit, but i would recommend using v9.0.0 or 9.0.0 instead of v9 for consistency. I believe other errors/warnings use the full version segments. Check around for how other errors/warn use the pattern.

At the point the breaking options are validated for the configuration, this value technically could have come from multiple places, config.json, config.ts, or cypress.config.js.

The file correct configFile is set here, which will use the project_utils's getDefaultConfigFilePath func to determine which cypress configuration file is found or it falls back to cypress.json.

Can we pass the configFie used into the error message as an additional argument?

@emilyrohrbough, good idea. I can make that flex.

emilyrohrbough · 2021-11-04T15:57:24Z

packages/server/lib/config.ts

-const validateNoBreakingConfig = (cfg) => {
-  return _.each(breakingOptions, ({ name, errorKey, newName, isWarning }) => {
-    if (_.has(cfg, name)) {
+const validateNoBreakingConfig = ({ config, configFile }) => {


The resolved configFile from option should be available on config. Should be able to pull like:

(cfg) = > { const { configFile } = cfg

indeed you are correct!

flotwig · 2021-11-04T17:31:43Z

packages/server/lib/util/find_system_node.js

-    })
-  })
-}
-
 module.exports = {
  findNodeInFullPath,


Ahh crap. Was gonna ask why this is here, but I forgot that findNodeInFullPath is used in the darwin child_process spawn workaround. We only use it in one place:

cypress/packages/launcher/lib/darwin/util.ts

Lines 108 to 119 in f8fd333

export async function darwinDetectionWorkaround (): Promise<FoundBrowser[]> {

const nodePath = await findSystemNode.findNodeInFullPath()

let args = ['./detection-workaround.js']

if (process.env.CYPRESS_INTERNAL_ENV === 'development') {

args = ['-r', '@packages/ts/register.js'].concat(args)

}

const { stdout } = await utils.execa(nodePath, args, { cwd: __dirname })

return JSON.parse(stdout)

}

The reason we need to use the system node here is a bug in Electron that causes cp.spawn to be very slow on darwin, which causes our browser detection to be very slow. I guess it's not an option to remove node-which entirely yet then since we'd have to make performance bad for global-mode users on darwin during browser detection.

flotwig · 2021-11-04T17:34:27Z

system-tests/test/system_node_spec.js

+      userNodePath: expectedNodePath,
+      userNodeVersion: expectedNodeVersion,


Wouldn't be too big of a change to system-tests.ts to add a useCli option, so that it spawns the cli instead of server. Would be useful for really fully system testing stuff like this. This is fine too for now.

It's a good idea but I don't want to tack it onto this PR. I created this issue: #18795 to track this request.

* Defaulting nodeVersion to system * try to fix system test * Rename arg parameters, fix system test in a much better way. * remove invalid comment * Add deprecation warning for the nodeVersion config. * Remove default value to avoid warning regardless of the presence of `nodeVersion` * More tests fixes 😅 * Updates to deprecation message * update node version in deprecation notice. * flex config file name that we tell consumers to update * simplify validateNoBreakingConfig options

Defaulting nodeVersion to system

f0c4585

mjhenkes requested a review from a team as a code owner November 1, 2021 18:17

mjhenkes requested review from jennifer-shehane and removed request for a team November 1, 2021 18:17

mjhenkes changed the title ~~Defaulting nodeVersion to system~~ feat: Defaulting nodeVersion to system Nov 1, 2021

jennifer-shehane removed their request for review November 1, 2021 18:28

try to fix system test

21815fb

brian-mann reviewed Nov 2, 2021

View reviewed changes

emilyrohrbough reviewed Nov 2, 2021

View reviewed changes

mjhenkes added 2 commits November 2, 2021 09:49

Rename arg parameters, fix system test in a much better way.

efabeb7

remove invalid comment

11b1d99

emilyrohrbough approved these changes Nov 2, 2021

View reviewed changes

mjhenkes added 3 commits November 2, 2021 10:33

Add deprecation warning for the nodeVersion config.

d2c3559

Remove default value to avoid warning regardless of the presence of `…

579bc5b

…nodeVersion`

More tests fixes 😅

72b9ca5

mjhenkes self-assigned this Nov 2, 2021

mjhenkes commented Nov 2, 2021

View reviewed changes

jennifer-shehane requested a review from brian-mann November 2, 2021 19:58

Updates to deprecation message

e33f3e7

mjhenkes commented Nov 3, 2021

View reviewed changes

brian-mann reviewed Nov 4, 2021

View reviewed changes

update node version in deprecation notice.

f202e14

flex config file name that we tell consumers to update

f50623e

flotwig self-requested a review November 4, 2021 15:37

emilyrohrbough reviewed Nov 4, 2021

View reviewed changes

simplify validateNoBreakingConfig options

f8fd333

flotwig approved these changes Nov 4, 2021

View reviewed changes

emilyrohrbough approved these changes Nov 4, 2021

View reviewed changes

mjhenkes merged commit 82429c0 into 9.0-release Nov 4, 2021

mjhenkes deleted the issue-18684 branch November 4, 2021 18:45

cypress-bot bot mentioned this pull request Nov 4, 2021

Have default nodeVersion be set to system instead of bundled #18684

Closed

mjhenkes mentioned this pull request Nov 4, 2021

feat: Node version schema update #18796

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Defaulting nodeVersion to system #18732

feat: Defaulting nodeVersion to system #18732

mjhenkes commented Nov 1, 2021 •

edited

Loading

cypress-bot bot commented Nov 1, 2021

cypress bot commented Nov 1, 2021 •

edited

Loading

brian-mann Nov 2, 2021

emilyrohrbough Nov 2, 2021

mjhenkes Nov 2, 2021

mjhenkes Nov 2, 2021

emilyrohrbough left a comment

brian-mann commented Nov 2, 2021

mjhenkes commented Nov 2, 2021

mjhenkes commented Nov 2, 2021

emilyrohrbough left a comment

jennifer-shehane commented Nov 2, 2021

mjhenkes Nov 2, 2021

mjhenkes Nov 2, 2021

mjhenkes Nov 3, 2021

brian-mann Nov 4, 2021

emilyrohrbough Nov 4, 2021 •

edited

Loading

mjhenkes Nov 4, 2021

emilyrohrbough Nov 4, 2021

mjhenkes Nov 4, 2021

flotwig Nov 4, 2021

flotwig Nov 4, 2021

mjhenkes Nov 4, 2021

	export async function darwinDetectionWorkaround (): Promise<FoundBrowser[]> {
	const nodePath = await findSystemNode.findNodeInFullPath()
	let args = ['./detection-workaround.js']

	if (process.env.CYPRESS_INTERNAL_ENV === 'development') {
	args = ['-r', '@packages/ts/register.js'].concat(args)
	}

	const { stdout } = await utils.execa(nodePath, args, { cwd: __dirname })

	return JSON.parse(stdout)
	}

		userNodePath: expectedNodePath,
		userNodeVersion: expectedNodeVersion,

feat: Defaulting nodeVersion to system #18732

feat: Defaulting nodeVersion to system #18732

Conversation

mjhenkes commented Nov 1, 2021 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Nov 1, 2021

cypress bot commented Nov 1, 2021 • edited Loading

Test summary

Run details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough left a comment

Choose a reason for hiding this comment

brian-mann commented Nov 2, 2021

mjhenkes commented Nov 2, 2021

mjhenkes commented Nov 2, 2021

emilyrohrbough left a comment

Choose a reason for hiding this comment

jennifer-shehane commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjhenkes commented Nov 1, 2021 •

edited

Loading

cypress bot commented Nov 1, 2021 •

edited

Loading

emilyrohrbough Nov 4, 2021 •

edited

Loading