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.

Sign up for GitHub

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: Node version schema update #18796

Merged
merged 2 commits into from
Nov 5, 2021
Merged

feat: Node version schema update #18796

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e79bb9b
Update Schema
mjhenkes Nov 4, 2021
1943211
update to mark as deprecated and restore the default.
mjhenkes Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions cli/schema/cypress.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,7 @@
"system",
"bundled"
],
"default": "bundled",
"description": "If set to 'system', Cypress will try to find a Node.js executable on your path to use when executing your plugins. Otherwise, Cypress will use the Node version bundled with Cypress."
"description": "If set to 'system', Cypress will use the node version that was used to launch the cli when executing your plugins. Otherwise, Cypress will use the Node version bundled with Cypress."
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this have 'system' as the default?

This description should be reversed also since the logic would apply if set to 'bundled'.

Suggested change
"description": "If set to 'system', Cypress will use the node version that was used to launch the cli when executing your plugins. Otherwise, Cypress will use the Node version bundled with Cypress."
"description": "If set to 'bundled', Cypress will use the Node version bundled with Cypress. Otherwise, Cypress will use the Node version that was used to launch the Cypress. This Node version is used when executing your plugins file and building spec files."

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically there is no default now because adding either system or bundled will trigger the deprecation warning.

https://github.com/cypress-io/cypress/blob/9.0-release/packages/server/lib/config_options.ts#L153-L155

I'm not sure exactly how to use this file, but I didn't want to encourage people to add the 'nodeVersion' option to config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually should I remove the nodeVersion entry entirely because it's deprecated?

Copy link
Member

Choose a reason for hiding this comment

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

But the deprecation warning is just a warning - the value still works. I don't understand the blocker for showing the default value as it is in the schema.

Copy link
Member

Choose a reason for hiding this comment

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

If things are deprecated they are still documented as before because the value is still completely functional, it just warns that it will be removed later.

Copy link
Member Author

Choose a reason for hiding this comment

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

We met in standup and decided to restore the default and mark the option as deprecated in the description.

},
"experimentalInteractiveRunEvents": {
"type": "boolean",
Expand Down