-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Node version schema update #18796
Conversation
Thanks for taking the time to open a PR!
|
cli/schema/cypress.schema.json
Outdated
@@ -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." |
There was a problem hiding this comment.
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'.
"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." |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Test summaryRun details
View run in Cypress Dashboard ➡️ Failures
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 |
* Update Schema * update to mark as deprecated and restore the default.
nodeVersion
be set tosystem
instead ofbundled
#18684User facing changelog
This is a follow-up PR to #18732 where I missed updating the cypress.schema.json file with the updated values. This PR should have the same change log as the previous PR.
Thanks for spotting this @emilyrohrbough !
Additional details
How has the user experience changed?
This change will not impact user experience.
PR Tasks
cypress.schema.json
? <-- whoops, I apparently missed this step on the last pr.