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

Expose an API to get the default config #5217

Closed
Aghassi opened this issue Sep 26, 2019 · 16 comments
Closed

Expose an API to get the default config #5217

Aghassi opened this issue Sep 26, 2019 · 16 comments

Comments

@Aghassi
Copy link

Aghassi commented Sep 26, 2019

We run Cypress through the API for our internal tooling, and have some config validation we wish to do. Right now, we use jest-validate to compare what is passed in to what is the "default". With other packages, like jest we can load their default config via jest-config. This makes it super easy to get the updates to the default config without having to add them manually.

We would like to do something similar with Cypress, but right now it seems this is the only file (above) that defines the config. It would be nice to have a cypress-config package that exposed a getDefault() function that gives this schema as an object which we can pass around and manipulate.

Is that something that would be possible?

@kuceb
Copy link
Contributor

kuceb commented Sep 26, 2019

@Aghassi do you need access in plugins (node) or support (browser, during test) ? We could probably do this pretty easily.

@kuceb
Copy link
Contributor

kuceb commented Sep 26, 2019

@Aghassi
Copy link
Author

Aghassi commented Sep 27, 2019

@bkucera the node API is how we are interfacing with triggering cypress, so preferably in node. I'll have a look at the example.

@Aghassi
Copy link
Author

Aghassi commented Sep 27, 2019

@bkucera It seems that although you set the defaults in that file, they are not exported in anyway that I could get them right? If there were a getDefaults function in there that I could use to get the CONFIG_DEFAULTS that would be helpful. Is that a change I could contribute/make?

@kuceb
Copy link
Contributor

kuceb commented Sep 27, 2019

@Aghassi yes, we would be open to a PR that exposes the defaults. We would need to think of the API for that, feel free to recommend how we should exposed it to the user (docs page on plugins/node API https://docs.cypress.io/api/plugins/writing-a-plugin.html#Plugins-API) attached to config? possibly third argument?

@Aghassi
Copy link
Author

Aghassi commented Sep 27, 2019

@bkucera this is my first time in the codebase, so I'm trying to get a mental model of how it compiles. It seems to be a monorepo, but the monorepo compiles into a single package called "cypress" yes? I guess, what I'm thinking is:

  • Take the constant you linked and make a separate package called @cypress/config that just exports that file as the main
  • Have @packages/server reference that file so it can still use the defaults
  • Publish @cypress/config as a standalone library that I can require or install so I can do const cypressDefaultConfig = require('@cypress/config').getDefault()
  • Update the docs to reflect that.

Does that make sense? I still don't fully understand the publishing model (though I have read the deploy docs) so I wanted to check if I was missing anything

@flotwig
Copy link
Contributor

flotwig commented Sep 27, 2019

@Aghassi The cli package is the source code for the actual cypress NPM module that gets distributed. Everything in packages/* gets bundled up with the Electron binary and downloaded at install time to be started by the cli.

Your solution could work, it would also force us to set up publishing for sub-repos which would be neat.

We could also just import and re-export cypress.schema.json in cli/lib/cypress.js to make it accessible. cypress.configSchema or something.

here's where we set defaults: /packages/server/lib/config.coffee@issue-2957#L81

side note, it would be cool if this could pull from cypress/cli/schema/cypress.schema.json - deduplication and all that

@kuceb
Copy link
Contributor

kuceb commented Sep 27, 2019

@Aghassi We have no precedence of exporting anything as a module except for the module API used to launch runs in node. So I was thinking something yielded to you in the module.exports = (on, config) => {} function instead of the imports. But your suggestion is one to consider as well

@kuceb
Copy link
Contributor

kuceb commented Sep 27, 2019

@ryan-snyder default config values can make the difference between a release being a feature vs being breaking. For this reason we need to control default configs in tandem with the cypress core release. Especially considering new cypress releases will introduce new config values.

There's an argument to be made for including subpackages such as cypress/config however

@Aghassi
Copy link
Author

Aghassi commented Sep 27, 2019

Ok, let me push a branch with my change while trying to keep what is existing. It may take a few iterations, but I understand the problem @bkucera is alluding to because we face it with our internal tooling too.

Aghassi added a commit to Aghassi/cypress that referenced this issue Sep 27, 2019
The goal of this PR is to help extract the default server
config into a separate package that can be consumed and published
standalone. If it is required that we move this out of the repo,
I'm fine with that to. The goal of the PR is to show a potential
solution for cypress-io#5217
@Aghassi
Copy link
Author

Aghassi commented Sep 27, 2019

@bkucera I opened a PR. It doesn't "work" so to speak, but it is a starter to show what I'm trying to get at. Ideally, the @packages/config would be something that we could publish to NPM anyway so that I could install it on my end and get the same config you guys use internally.

@Aghassi
Copy link
Author

Aghassi commented Oct 12, 2019

@bkucera Just wanted to circle back on this and see if I could get some feedback on that PR. Would like to see if we can drive this to completion 😄

@sainthkh
Copy link
Contributor

I close this issue because it is not necessary to the most users. The PR related to this issue is closed and this issue didn't get any thumbs-up.

If anyone really wants this feature, please open a new issue.

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

No branches or pull requests

5 participants
@flotwig @Aghassi @sainthkh @kuceb and others