chore: refactor server/lib/config.ts to pkg/config #22530

sainthkh · 2022-06-27T07:28:27Z

Part 1 of Refactor config logic in server to the config package #21781

User facing changelog

N/A. Internal refactoring.

Additional details

Why was this change necessary? => Having them in pkg/server makes us create configApi inside DataContext and not be able to call them directly.
What is affected by this change? => None. It's a just code cleanup.

Any implementation details to explain?

At first, I thought this issue would be a simple copy and paste of a few lines of code. But I learned that it's a lot bigger than I thought.

To refactor server/lib/config.ts, I had to refactor these files, too:

server/lib/util/config.ts
server/lib/util/coerce.ts
server/lib/util/keys.ts
server/lib/util/origin.ts
server/lib/util/path_helpers.ts

Because they're used mostly inside config.ts and related with manipulating config values.

As we're migrating our code from commonjs to esm, the code structure also had to be fixed.

For this PR, I created 3 files to place the code.

config/src/project/index.ts -> The public api or the project config functions used outside the config package. I named them project because the type definitions were in ProjectConfigManager, ProjectLifecycleManager.
config/src/project/util.ts -> The internal functions to be used to implement the public api at index.ts. They are exported to be unit-tested.
config/src/util.ts -> Some utility functions like hideKeys and coerce are used outside pkg/config. They live here.

As for origin.ts, I moved it to pkg/network/lib/uri.ts because it more suits the place.

I renamed some functions to make it clear.

keys -> hideKeys
isDefault -> isResolvedConfigPropDefault

I added types for the functions. I added some any because I'm not sure about them now.

Steps to test

No new features; ensure existing tests are passing.

How has the user experience changed?

N/A. It's a code cleanup.

PR Tasks

Have tests been added/updated?
[na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?

cypress-bot · 2022-06-27T07:28:29Z

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 · 2022-07-12T23:46:47Z

Test summary

37848 • 0 • 469 • 0 • 12

Run details


Project	cypress
Status	Passed
Commit	`e4e681c`
Started	Aug 5, 2022 2:35 AM
Ended	Aug 5, 2022 2:57 AM
Duration	21:44 💡
OS	Linux Debian - 11.3
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	net_stubbing.cy.ts	1
1	network stubbing > intercepting request > can modify the request body
	xhr.cy.js	4
1	... > logs request + response headers
2	... > logs Method, Status, URL, and XHR
3	... > logs response
4	... > logs request + response headers
This comment includes only the first 5 flaky tests. See all 12 flaky tests in the 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

emilyrohrbough · 2022-07-13T01:38:27Z

packages/config/src/project/utils.ts

+import _ from 'lodash'
+import path from 'path'
+
+import { getProcessEnvVars, CYPRESS_SPECIAL_ENV_VARS } from '@packages/server/lib/util/config'


Any way we can drop pulling in @packages/server or @packages/data-context?

I'm trying. I guess we can remove @package/server from here.

So excited to see these changes!

sainthkh · 2022-07-18T02:53:04Z

packages/config/src/project/index.ts

+import deepDiff from 'return-deep-diff'
+
+import errors, { ConfigValidationFailureInfo, CypressError } from '@packages/errors'
+import { getCtx } from '@packages/data-context/src/globalContext'


This dependency cannot be removed until we move getFilesByGlob to somewhere else.

Or call it inside data-context and pass the value to this function.

Maybe, this can be solved in the future PRs.

I researched it after writing this comment. It seems that it will work if we add supportFiles member to setupFullConfigWithDefaults.

I'll do this after this PR is merged. This PR is big and I tried not to change code much.

sainthkh · 2022-07-18T02:59:50Z

packages/config/src/project/index.ts

+
+const debug = Debug('cypress:config:project')
+
+// TODO: any -> SetupFullConfigOptions in data-context/src/data/ProjectConfigManager.ts


Using data-context/src/data/ProjectConfigManager.ts type definitions breaks the type. It seems that some types are missing. I postponed it for now because I need to read a lot of code to be sure that the new types are correct.

sainthkh · 2022-07-18T03:02:17Z

packages/data-context/src/globalContext.ts

@@ -0,0 +1,49 @@
+import type { DataContext } from './DataContext'


I created this file to bypass circular dependency.

Where were you seeing this at?

pkg/config uses getCtx and pkg/data-context uses pkg/config functions. It causes circular dependency and breaks Cypress.

This file could be removed and merged back to index.ts after fixing the problems I mentioned in this comment.

@sainthkh I think this can be removed now with your latest changes!

Unfortunately, it can't. As I wrote in the comment below, we need getCtx from pkg/data-context dependency in tests. Check this code

I thought about injection dummy functions to the test. But its return value is used to throw errors. So, it can cause false positive when getFilesByGlob is changed.

I think the only way to roll back this code is to:

solve that obj.supportFile issue

pass supportFile from pkg/data-context

remove pkg/data-context from tests in pkg/config

remove this globalContext file.

able to delete now with the latest commits?

sainthkh · 2022-07-18T03:07:20Z

packages/network/lib/uri.ts

+const parseClone = (urlObject: any) => {
 return url.parse(_.clone(urlObject))
 }


This is really weird code. url.parse expects string as its first arg, but our code is written for the case that it's not a string but an object.

I'll check this behavior and fix this in the part 2 of this PR.

sainthkh · 2022-07-18T03:08:08Z

packages/server/lib/config.ts

-import type { ConfigValidationFailureInfo, CypressError } from '@packages/errors'
+export const setupFullConfigWithDefaults = configUtils.setupFullConfigWithDefaults

-import { getCtx } from './makeDataContext'
+export const updateWithPluginValues = configUtils.updateWithPluginValues


These are here because they're used in some tests in pkg/server

sainthkh · 2022-07-18T03:12:27Z

packages/server/test/unit/config_spec.js

- expect(errors.throwErr).not.to.be.called
- })
- })
-
 context('.get', () => {


The tests under .get more suit pkg/data-context/test/unit/data/ProjectLifecycleManager.

If you agree, I'll move them in the next PR.

sainthkh · 2022-07-18T03:13:36Z

packages/ts/tsconfig.json

@@ -3,7 +3,7 @@
 /* Basic Options */
 "target": "ES2018", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', or 'ESNEXT'. */
 "module": "commonjs", /* Specify module code generation: 'commonjs', 'amd', 'system', 'umd' or 'es2015'. */
- "lib": ["es2018", "ES2020.Promise"], /* Specify library files to be included in the compilation: */
+ "lib": ["es2018", "ES2020.Promise", "ES2021.String"], /* Specify library files to be included in the compilation: */


String.ReplaceAll is used in several pacakges. So, I added it here to avoid surprise.

lmiller1990

Glad to see the slow march from CJS+JS to TS, and gradual improvements to type safety. This generally looks like a pretty straight forward refactor - I don't see any obvious problems, just made a few recommendations around (potential) type safety improvements.

lmiller1990 · 2022-07-19T00:23:38Z

packages/config/src/project/index.ts

+ config.projectName = projectName
+
+ // @ts-ignore
+ return mergeDefaults(config, options, cliConfig)


How come we need to ts-ignore here?

It's because of the many errors inside and outside mergeDefaults like these:

I'll check if I can add those types to the definitions and fix them in the subsequent PR.

Alright, I figured it was something like this.

packages/network/lib/uri.ts

packages/config/src/project/index.ts

emilyrohrbough · 2022-07-21T17:55:37Z

packages/config/src/project/index.ts

+import deepDiff from 'return-deep-diff'
+
+import errors, { ConfigValidationFailureInfo, CypressError } from '@packages/errors'
+import { getCtx } from '@packages/data-context/src/globalContext'


We are calling this from the data-context package. Can we drop this dependency and just pass in the configFile as an additional parameter to updateWithPluginValues?

getCtx is called in 2 functions:

updateWithPluginValues of project/index.ts

setSupportFileAndFolder of project/utils.ts

The solution for the first case is simple. We can simply add configFile member inside buildBaseFullConfig of ProjectConfigManager.

But the problem is the second one.

To call the ctx.file.getFilesByGlob, we need obj.supportFile. But the problem is that global supportFile option is legacy. It is splitted and moved under e2e or component from 10.

It means that current setSupportFileAndFolder can be buggy or completely removable in the current version. I'm not 100% sure about this. I need to think about a test case that shows this problem. Maybe #22776 is the related case.

The goal of this PR is copying and moving server/lib/config.ts to pkg/config and adding types as much as possible. I focused on this because this PR can be too big and it makes reviewers hard to review this PR.

I think checking/fixing bugs and adding more specific types should be done in the future PRs. What do you think?

To call the ctx.file.getFilesByGlob, we need obj.supportFile. But the problem is that global supportFile option is legacy. It is splitted and moved under e2e or component from 10.

This log is correct, at this point we are testing the flattened configuration for the given testing type, so obj.supportFile is valid.

I think making a few small changes seems needed to move this code over and would be worth making a small change to remove this circular dependency. Simply passing int he attribute needed is probably the simplest approach until / if we ever want to revisit how this is composed.

// data-context/ProjectConfigManger.ts setupFullConfigWithDefaults({ projectRoot, projectName, configFile, config, envFile, options, cliConfig }, ctx.file.getFilesByGlob)

I never thought about passing the function as an argument to remove the dependency. Thanks for the idea.

Unfortunately, this idea even cannot completely remove pkg/data-context from config. It can be removed from src. But it had to be added to test to run tests.

However, I think this change is much better.

packages/server/lib/makeDataContext.ts

packages/server/package.json

packages/server/lib/util/args.js

emilyrohrbough · 2022-07-21T18:01:35Z

packages/data-context/src/globalContext.ts

@@ -0,0 +1,49 @@
+import type { DataContext } from './DataContext'


Where were you seeing this at?

They're only used inside config.ts

Conflicts: packages/server/test/unit/config_spec.js

rachelruderman · 2022-07-29T15:45:35Z

Hi @sainthkh ! Can you update Steps to test in the description? I realize this is a refactor PR so it may be as simple as "No new features; ensure existing tests are passing"

sainthkh · 2022-08-02T01:20:21Z

packages/config/test/project/utils.spec.ts

@@ -0,0 +1,1182 @@
+import '@packages/server/test/spec_helper'
+import { getCtx } from '@packages/data-context/src/globalContext'


We need globalContext here. Otherwise, we need to import the entire pkg/data-context.

sainthkh · 2022-08-02T01:25:18Z

packages/config/src/project/utils.ts

+ const supportFilesByGlob = await getFilesByGlob(obj.projectRoot, obj.supportFile)
+
+ if (supportFilesByGlob.length > 1) {
+ return errors.throwErr('MULTIPLE_SUPPORT_FILES_FOUND', obj.supportFile, supportFilesByGlob)
+ }
+
+ if (supportFilesByGlob.length === 0) {
+ if (obj.resolved.supportFile.from === 'default') {
+ return errors.throwErr('DEFAULT_SUPPORT_FILE_NOT_FOUND', relativeToProjectRoot(obj.projectRoot, obj.supportFile))
+ }
+
+ return errors.throwErr('SUPPORT_FILE_NOT_FOUND', relativeToProjectRoot(obj.projectRoot, obj.supportFile))
+ }
+


Return value is used to throw errors.

packages/config/test/project/utils.spec.ts

rachelruderman

@sainthkh looks like we have a failing server performance test:

sainthkh · 2022-08-15T08:36:09Z

@rachelruderman For some reason, context was cleared before before in proxy_performance_spec. I solved the problem by adding new context before calling getCtx. I didn't create stub here because we're testing performance. Creating stub can make test good-for-nothing test result.

rachelruderman

Looks great, thank you for your contribution @sainthkh !

sainthkh force-pushed the issue-21781 branch 2 times, most recently from a75a5e9 to d5e2ebe Compare July 12, 2022 23:23

emilyrohrbough reviewed Jul 13, 2022

View reviewed changes

sainthkh force-pushed the issue-21781 branch 2 times, most recently from a3c694f to 1d14c2b Compare July 18, 2022 01:41

sainthkh commented Jul 18, 2022

View reviewed changes

sainthkh marked this pull request as ready for review July 18, 2022 03:16

sainthkh requested review from a team as code owners July 18, 2022 03:16

sainthkh requested review from rachelruderman and emilyrohrbough and removed request for a team July 18, 2022 03:16

lmiller1990 approved these changes Jul 19, 2022

View reviewed changes

sainthkh force-pushed the issue-21781 branch from 565219b to 6a37a5d Compare July 19, 2022 07:11

emilyrohrbough requested changes Jul 21, 2022

View reviewed changes

sainthkh added 7 commits July 22, 2022 08:45

test

aa59a6b

fix

99992ed

Migrate origin.js to pkg/network.

187f955

Migrate util/keys.js, path_helpers.js to pkg/config.

0e022e1

They're only used inside config.ts

Migrate config.ts to pkg/config.

1b123ea

fix

5dfd312

fix

a9fc499

feedback

6a7d5b7

sainthkh force-pushed the issue-21781 branch from 6a37a5d to 6a7d5b7 Compare July 22, 2022 01:22

sainthkh added 3 commits July 29, 2022 10:37

Merge branch 'develop'

9eac66f

Conflicts: packages/server/test/unit/config_spec.js

remove pkg/data-context from pkg/config/src.

b387035

Fix tests.

c110394

Merge branch 'develop' into issue-21781

9752697

sainthkh commented Aug 2, 2022

View reviewed changes

emilyrohrbough reviewed Aug 3, 2022

View reviewed changes

packages/config/test/project/utils.spec.ts Outdated Show resolved Hide resolved

sainthkh added 4 commits August 4, 2022 11:26

Merge branch 'develop' into issue-21781

33947ff

remove data-context

be35d7a

remove globalContext file

8e59c4c

Merge remote-tracking branch 'upstream/develop' into issue-21781

e4e681c

emilyrohrbough approved these changes Aug 5, 2022

View reviewed changes

Merge branch 'develop' into issue-21781

9054ea0

rachelruderman suggested changes Aug 10, 2022

View reviewed changes

sainthkh added 3 commits August 15, 2022 16:42

Fix merge conflicts.

2e811a2

Copy develop/packages/data-context/src/index.ts

0153307

fix test failure.

8fdb994

Merge branch 'develop' into issue-21781

fd0f915

emilyrohrbough requested a review from rachelruderman August 16, 2022 18:01

rachelruderman and others added 4 commits August 17, 2022 08:34

Merge branch 'develop' into issue-21781

551ff00

Delete zombie code that snuck in with merge conflict

917416f

add experimentalSingleTabRunMode

d2af72f

Merge branch 'develop' into issue-21781

ac3da44

rachelruderman approved these changes Aug 17, 2022

View reviewed changes

rachelruderman merged commit a27affe into cypress-io:develop Aug 17, 2022

MikeMcC399 mentioned this pull request Sep 9, 2024

docs: replace debug log source cypress:server:specs cypress-io/cypress-documentation#5920

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor server/lib/config.ts to pkg/config #22530

chore: refactor server/lib/config.ts to pkg/config #22530

sainthkh commented Jun 27, 2022 •

edited

Loading

cypress-bot bot commented Jun 27, 2022

cypress bot commented Jul 12, 2022 •

edited

Loading

emilyrohrbough Jul 13, 2022

sainthkh Jul 13, 2022

emilyrohrbough Jul 13, 2022

sainthkh Jul 18, 2022

sainthkh Jul 18, 2022

sainthkh Jul 18, 2022

sainthkh Jul 18, 2022

emilyrohrbough Jul 21, 2022

sainthkh Jul 22, 2022

emilyrohrbough Aug 1, 2022

sainthkh Aug 2, 2022

emilyrohrbough Aug 4, 2022

sainthkh Jul 18, 2022

sainthkh Jul 18, 2022

sainthkh Jul 18, 2022

sainthkh Jul 18, 2022

lmiller1990 left a comment

lmiller1990 Jul 19, 2022

sainthkh Jul 19, 2022 •

edited

Loading

lmiller1990 Jul 19, 2022

emilyrohrbough Jul 21, 2022

sainthkh Jul 22, 2022

emilyrohrbough Jul 27, 2022

sainthkh Jul 29, 2022

emilyrohrbough Jul 21, 2022

rachelruderman commented Jul 29, 2022

sainthkh Aug 2, 2022

sainthkh Aug 2, 2022

rachelruderman left a comment

sainthkh commented Aug 15, 2022

rachelruderman left a comment


		const debug = Debug('cypress:config:project')

		// TODO: any -> SetupFullConfigOptions in data-context/src/data/ProjectConfigManager.ts

		@@ -0,0 +1,49 @@
		import type { DataContext } from './DataContext'

		@@ -0,0 +1,1182 @@
		import '@packages/server/test/spec_helper'
		import { getCtx } from '@packages/data-context/src/globalContext'

chore: refactor server/lib/config.ts to pkg/config #22530

chore: refactor server/lib/config.ts to pkg/config #22530

Conversation

sainthkh commented Jun 27, 2022 • edited Loading

User facing changelog

Additional details

Any implementation details to explain?

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Jun 27, 2022

cypress bot commented Jul 12, 2022 • edited Loading

Test summary

Run details

Flakiness

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sainthkh Jul 19, 2022 • 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

rachelruderman commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachelruderman left a comment

Choose a reason for hiding this comment

sainthkh commented Aug 15, 2022

rachelruderman left a comment

Choose a reason for hiding this comment

sainthkh commented Jun 27, 2022 •

edited

Loading

cypress bot commented Jul 12, 2022 •

edited

Loading

sainthkh Jul 19, 2022 •

edited

Loading