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

[WIP] export default configuration on moduleAPI #5229

Closed

Conversation

Aghassi
Copy link

@Aghassi Aghassi commented 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 #5217

Pre-merge Tasks

  • Have tests been added/updated for the changes in this PR?
  • Has a PR to cypress-documentation been submitted to document any user-facing changes?
  • Have the type definitions been updated with any user-facing API changes?
  • Has the cypress.schema.json been updated with any new configuration options?
  • Has the original issue been tagged with a release in ZenHub?

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
@CLAassistant
Copy link

CLAassistant commented Sep 27, 2019

CLA assistant check
All committers have signed the CLA.

@kuceb
Copy link
Contributor

kuceb commented Oct 12, 2019

so that it can be consumed and published
standalone

sorry I'm not understanding, what exactly is the end goal to the user? Could you mention some use cases this supports? possibly a simpler solution is just moving it to another file/exporting it so users can use import {defaultConfig} from 'cypress/config'

@Aghassi
Copy link
Author

Aghassi commented Oct 13, 2019

@bkucera The use case (for me, and hopefully others) is if I want to import the default ruleset, I can. In our case, we provide a default ruleset which we do two things with via our tooling:

  • Allow overrides by validating with jest-validate
  • provide defaults that people can use without configuration such that it works in our system

The goal here being that I can properly provide a default without having to manually amend the config every time there is a Cypress update in the event that the config gets a new key added or a key is removed.

@kuceb
Copy link
Contributor

kuceb commented Oct 15, 2019

@Aghassi does #5218 support your use case? moving allow .js config files

@Aghassi
Copy link
Author

Aghassi commented Oct 15, 2019

@bkucera That's part of it, but we already are doing this internally since we feed the NodeJS API parameters instead of using the CLI directly and having it find a config.

@kuceb
Copy link
Contributor

kuceb commented Oct 15, 2019

@Aghassi alright, so it looks like if we export the defaults as part of the module api like so: import Cypress, { defaultConfig } from 'cypress' this would satisfy your use case.

Is that correct?

@Aghassi
Copy link
Author

Aghassi commented Oct 15, 2019

@bkucera That is correct 😄

@kuceb
Copy link
Contributor

kuceb commented Oct 16, 2019

@Aghassi sounds like a manageable PR, shouldn't require making a new package.json, if you want to get the ball rolling on this you could implement that in this PR

@Aghassi
Copy link
Author

Aghassi commented Oct 16, 2019

@bkucera Sounds good. Something that was confusing to me was how the current package compiles into the final cypress package so I can export the module. Can you help me understand how that works? Once I understand that, it should be a fairly easy change

@kuceb
Copy link
Contributor

kuceb commented Oct 16, 2019

@Aghassi this should be it: https://github.com/cypress-io/cypress/blob/develop/cli/lib/cypress.js

However we don't export a default member, so probably just add it to the moduleApi obj

@jennifer-shehane jennifer-shehane requested review from kuceb and removed request for kuceb October 16, 2019 18:42
@jennifer-shehane jennifer-shehane changed the title feat: move default server configs to a separate package WIP: move default server configs to a separate package Oct 16, 2019
@jennifer-shehane
Copy link
Member

Hey @Aghassi, I've added WIP to the title of this PR. Please remove this from the PR when it is ready to be reviewed and ping us for another review. Thanks.

@kuceb kuceb changed the title WIP: move default server configs to a separate package [WIP] move default server configs to a separate package Oct 16, 2019
@kuceb kuceb changed the title [WIP] move default server configs to a separate package [WIP] export default configuration to moduleAPI Oct 16, 2019
@kuceb kuceb changed the title [WIP] export default configuration to moduleAPI [WIP] export default configuration on moduleAPI Oct 16, 2019
@Aghassi
Copy link
Author

Aghassi commented Oct 19, 2019

Thanks. So if I follow the structure of this repo @bkucera, all packages/ are private because they end up in the final binary. Because of this, the cli/ directory can't access them currently. I see three solutions to being able to achieve that module export, but wanted guidance on the "best" one.

  1. Convert the existing coffee script config file to javascript and have one in the server package and one in the cli folder (I don't like this because of duplication)
  2. Make the server package public, and publish it to NPM. Then have that cli depend on it and import it (I do like this one, but requires adding some extra build steps to the pipeline).
  3. Extract out the coffee config object into a separate file like defaultCypressConfig.js which the config.coffee file references via a require, but which also is copied during build time to the cli folder so we can module export it.

Do any of these sound better than the others to you? I like 3 because it is simple and doesn't really disrupt much of the build flow you currently have.

@kuceb
Copy link
Contributor

kuceb commented Oct 23, 2019

@Aghassi now that I thought more about this, we try to keep the cli as thin and dumb as possible, and copying a default config file into both the binary and cli breaks that goal. Currently, the cli has no knowledge of valid config/cli args, it just passes them along. We could however, yield you the defaults in the plugins api, e.g.:

// plugins/index.js
module.exports = (on, config) => {
  config.defaults.defaultCommandTimeout
}

I know you said you want access through the moduleApi, but would this work for you? that would also make this PR go in much faster

@Aghassi
Copy link
Author

Aghassi commented Oct 24, 2019

@bkucera Not really as we would like to validate the config before Cypress starts :/. Kind of like how webpack is able to validate their config before webpack starts. Same with Jest. Once Cypress is running, we don't have much control on config validation.

@kuceb
Copy link
Contributor

kuceb commented Oct 24, 2019

@Aghassi good point, maybe a shared subpackage that's copied into both cli/binary is the best solution here. I know it will take some work to get that right however.

Here's where we build the final binary, that could possible copy over a shared/common dir, or have the cli do it at build time

https://github.com/cypress-io/cypress/blob/develop/scripts/binary/build.coffee

@jennifer-shehane
Copy link
Member

@Aghassi Will you have time to finish this PR? If not, we will close this PR due to inactivity.

@Aghassi
Copy link
Author

Aghassi commented Dec 20, 2019

@jennifer-shehane I will, I have been a little busy with work and have not had a chance to. I will try and get some of it done this weekend. Have to brush up on this repo's build cycle again to see where I can accomplish the copy over.

@jennifer-shehane
Copy link
Member

@Aghassi Will you have time to finish this PR? If not, we will close this PR due to inactivity.

@Aghassi Aghassi closed this Feb 18, 2020
@Aghassi
Copy link
Author

Aghassi commented Feb 18, 2020

I've since moved on to another team. @krohrsb can take point on this in the future if that team still requires it.

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

Successfully merging this pull request may close these issues.

4 participants