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

chore: enable caching for lint task #26791

Merged
merged 13 commits into from
Jun 2, 2023
Merged

chore: enable caching for lint task #26791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
872833e
chore: add empty nx.json [run ci]
jordanpowell88 May 9, 2023
03e67a0
chore: add lint caching [run ci]
jordanpowell88 May 17, 2023
c18ff43
chore: add target defaults and namedInputs
jordanpowell88 May 18, 2023
a0152b7
chore: remove sharedGlobals
jordanpowell88 May 22, 2023
3b18ca4
chore: add devDeps to cli
jordanpowell88 May 22, 2023
7bce65e
chore: update build-npm-modules script
jordanpowell88 May 22, 2023
0c6b748
chore: update build-npm-modules script
jordanpowell88 May 22, 2023
bf670a3
chore: add build global dependsOn
jordanpowell88 May 22, 2023
2f86a75
chore: add ts-ignore comment
jordanpowell88 May 23, 2023
764e961
chore: update build-prod targetDefaults
jordanpowell88 May 23, 2023
542a38d
chore: update build-npm-modules script
jordanpowell88 May 22, 2023
a66a89c
chore: update build-npm-modules script
jordanpowell88 May 22, 2023
42aefb1
chore: remove no-console rule in root eslintrc
jordanpowell88 Jun 1, 2023
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
1 change: 0 additions & 1 deletion .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ module.exports = {
rules: {
'no-restricted-properties': 'off',
'no-restricted-syntax': 'off',
'no-console': 'off',
},
},
],
Expand Down
8 changes: 8 additions & 0 deletions cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,15 @@
"devDependencies": {
"@babel/cli": "7.13.0",
"@babel/preset-env": "7.13.5",
"@cypress/angular": "0.0.0-development",
"@cypress/grep": "0.0.0-development",
"@cypress/mount-utils": "0.0.0-development",
"@cypress/react": "0.0.0-development",
"@cypress/react18": "0.0.0-development",
"@cypress/sinon-chai": "2.9.1",
"@cypress/svelte": "0.0.0-development",
"@cypress/vue": "0.0.0-development",
"@cypress/vue2": "0.0.0-development",
"@packages/root": "0.0.0-development",
"@types/bluebird": "3.5.33",
"@types/chai": "4.2.15",
Expand Down
3 changes: 2 additions & 1 deletion lerna.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
"packages/*",
"npm/*",
"tooling/*",
"system-tests"
"system-tests",
"scripts"
],
"useWorkspaces": true,
"useNx": true,
Expand Down
1 change: 1 addition & 0 deletions npm/angular/src/mount.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ export type MountResponse<T> = {
// 'zone.js/testing' is not properly aliasing `it.skip` but it does provide `xit`/`xspecify`
// Written up under https://github.com/angular/angular/issues/46297 but is not seeing movement
// so we'll patch here pending a fix in that library
// @ts-ignore Ignore so that way we can bypass semantic error TS7017: Element implicitly has an 'any' type because type 'typeof globalThis' has no index signature.
globalThis.it.skip = globalThis.xit

@Injectable()
Expand Down
41 changes: 39 additions & 2 deletions nx.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,46 @@
"default": {
"runner": "nx-cloud",
"options": {
"cacheableOperations": [],
"cacheableOperations": [
"lint"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is what enables lint tasks to be cached with Nx Cloud

],
"accessToken": "ZmNlNjA0YzAtNTM1NS00MDIwLWFlMWItNWYxYzNiMjQ4N2VkfHJlYWQtb25seQ=="
}
}
},
"targetDefaults": {
"lint": {
"inputs": [
"default",
"{workspaceRoot}/.eslintrc.js"
]
},
"build": {
"dependsOn": [
"^build"
]
},
"build-prod": {
"dependsOn": [
"^build-prod"
]
}
},
"namedInputs": {
"sharedGlobals": [],
"default": [
"{projectRoot}/**/*",
"sharedGlobals"
],
"production": [
"default",
"!{projectRoot}/**/*.spec.ts",
"!{projectRoot}/**/*.md",
"!{projectRoot}/tsconfig.spec.json",
"!{projectRoot}/.eslintrc.json",
"!{projectRoot}/.mocharc.{js,json}",
"!{projectRoot/cypress.config.{ts,js}",
"!{projectRoot/**/*.cy.ts"
]
Comment on lines +13 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

@jordanpowell88 I am not sure how NX works exactly, so I am not sure how to validate the changes here in nx.json. Can you describe what each part is doing and point to NX documentation that describes how these should be configured. I am guessing it is describing what should or should not be cache for the task, but not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@warrensplayer . Here is the documentation.

But yes in short, yes the namedInputs are used to define sort of "shared" global things that specific tasks would read to determine if a library's cache should be invalidated or not.

For Example: the defaultTargets.lint.inputs array has 2 items that invalidate it's cache. The workspace .eslintrc.js file and default. Default points to the namedInputs.default array which includes any file in that libraries directory ({projectRoot}/**/*) and the sharedGlobals which would be any shared global files that should be included across tasks (in our case it is empty)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. Final question in this section... Is the production option under namedInputs being used by anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently. But I went ahead and added what I would expect to be starting values we would use. We can certainly remove this if we want for the time being though

}
}
}
7 changes: 4 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"binary-upload": "node ./scripts/binary.js upload",
"binary-zip": "node ./scripts/binary.js zip",
"build": "yarn build-npm-modules && lerna run build --stream --no-bail --ignore create-cypress-tests --ignore cypress --ignore \"'@packages/{runner}'\" --ignore \"'@cypress/{angular,react,react18,vue,vue2,mount-utils,svelte}'\" && node ./cli/scripts/post-build.js && lerna run build --stream --scope create-cypress-tests",
"build-npm-modules": "lerna run build --scope cypress --scope @cypress/mount-utils && lerna run build --scope \"'@cypress/{angular,react,react18,vue,vue2,svelte}'\"",
"build-npm-modules": "lerna run build --scope cypress --scope @cypress/mount-utils --scope @cypress/react && lerna run build --scope \"'@cypress/{angular,react18,vue,vue2,svelte}'\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jordanpowell88 What is this change doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically because npm/react18 dependsOn npm/react this was failing in CI. It's the same reason why we are building npm/mount-utils before building the rest. By adding Nx into the equation and because we have 2 outputs for the npm libs (their dist and the cli) it was sometimes creating an issue in CI. This change appears to have fixed the inconsistency in CI

Copy link
Contributor

Choose a reason for hiding this comment

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

How did your changes for the lint task cause this task to start failing? I saw the issue you had in this branch in CI, but could not find it failing in develop. Also, I was looking at the differences between the build step in this branch and develop and saw a few differences. The main one was this:
cypress:build --> @cypress/react:build --> cypress:build
here: https://app.circleci.com/pipelines/github/cypress-io/cypress/53051/workflows/046ba13e-d424-469d-a1af-aeac31bc00ed/jobs/2186964/parallel-runs/0/steps/0-114

Is this a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea we have many circular dependencies which is causing this. That is why it requires a bit of change to the build step even though we aren't technically doing anything with build in this PR. This was work that would need to be done either eventually anyways when we cache the build step

"build-prod": "lerna run build-prod-ui --stream && lerna run build-prod --stream --ignore create-cypress-tests && node ./cli/scripts/post-build.js && lerna run build-prod --stream --scope create-cypress-tests --scope",
"build-v8-snapshot-dev": "node --max-old-space-size=8192 tooling/v8-snapshot/scripts/setup-v8-snapshot-in-cypress.js --env=dev",
"build-v8-snapshot-prod": "node --max-old-space-size=8192 tooling/v8-snapshot/scripts/setup-v8-snapshot-in-cypress.js",
Expand Down Expand Up @@ -41,7 +41,7 @@
"ensure-deps": "./scripts/ensure-dependencies.sh",
"get-next-version": "node scripts/get-next-version.js",
"postinstall": "node ./scripts/run-postInstall.js",
"lint": "lerna run lint --no-bail --concurrency 2 && eslint --ext .js,.ts,.json, scripts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the linting of the /scripts folder is now moved inside of /scripts/package.json

Copy link
Member

Choose a reason for hiding this comment

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

this dropped linting of the root-level js/json files which we want to keep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The root level files were not being linted before. We were only running lint across all the packages + manually linting the scripts folder. This just moves the manual linting of the scripts to it's own lint job so the first command will trigger it

"lint": "lerna run lint --no-bail --concurrency 2",
"prepare-release-artifacts": "node ./scripts/prepare-release-artifacts.js",
"npm-release": "node scripts/npm-release.js",
"prestart": "yarn ensure-deps",
Expand Down Expand Up @@ -247,7 +247,8 @@
"packages/*",
"npm/*",
"tooling/*",
"system-tests"
"system-tests",
"scripts"
],
"nohoist": [
"**/webpack-preprocessor/babel-loader",
Expand Down
17 changes: 17 additions & 0 deletions scripts/.eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to re-use the root-level eslint configuration instead of needing to manage another one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the root .eslintrc config will throw 340 lint errors:

  • Unexpected console statement
  • Synchronous fs calls should not be used in Cypress

We could certainly ignore these other ways but I feel like a separate config for these scripts makes sense given these warnings. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

What changed that is is failing with this PR using the same eslint config file and not in develop?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jordanpowell88 I found this PR here from a few months ago that was adding linting to the scripts directory. In it, a no-console of off was added to an overrides section in the main .eslintrc.js file. Should this now be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@warrensplayer great catch! I removed that rule in 42aefb1

"plugins": [
"cypress",
"@cypress/dev"
],
"extends": [
"plugin:@cypress/dev/general",
"plugin:@cypress/dev/tests"
],
"env": {
"cypress/globals": true
},
"rules": {
"no-console": "off",
"prefer-rest-params": "off"
}
}
6 changes: 6 additions & 0 deletions scripts/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"name": "internal-scripts",
"scripts": {
"lint": "eslint --ext .js,.ts,.json, ."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The root level package.json lint script also ran this task. This makes the /scripts directory its own lib from an Nx standpoint and moves the lint script here so that it can be cached the same way the other libs do

}
}