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

fix: fix third party reporters and cy.origin calls in Cypress #24614

Closed
wants to merge 10 commits into from
Closed

fix: fix third party reporters and cy.origin calls in Cypress #24614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6d1af54
fix: improve binary cleanup to look at binary rather than monorepo
ryanthemanuel Nov 8, 2022
efd0af7
fix rewriter
ryanthemanuel Nov 8, 2022
8e22bad
Additional fixes
ryanthemanuel Nov 9, 2022
cc95def
Merge branch 'develop' into ryanm/fix/improve-binary-cleanup-to-look-…
ryanthemanuel Nov 9, 2022
f236a3d
fix windows
ryanthemanuel Nov 9, 2022
4912d23
fix issue with errors and babel/runtime
ryanthemanuel Nov 9, 2022
e5fe1e9
fix windows build
ryanthemanuel Nov 9, 2022
46333d6
Merge branch 'develop' into ryanm/fix/improve-binary-cleanup-to-look-…
ryanthemanuel Nov 9, 2022
c01362f
fix windows
ryanthemanuel Nov 9, 2022
a77d4f0
fix windows
ryanthemanuel Nov 9, 2022
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
10 changes: 5 additions & 5 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ mainBuildFilters: &mainBuildFilters
branches:
only:
- develop
- 'zachw/fix-vite-hoisting-issue'
- 'ryanm/fix/improve-binary-cleanup-to-look-at-binary-rather-than-monorepo'

# usually we don't build Mac app - it takes a long time
# but sometimes we want to really confirm we are doing the right thing
Expand All @@ -36,7 +36,7 @@ macWorkflowFilters: &darwin-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'zachw/fix-vite-hoisting-issue', << pipeline.git.branch >> ]
- equal: [ 'ryanm/fix/improve-binary-cleanup-to-look-at-binary-rather-than-monorepo', << pipeline.git.branch >> ]
- matches:
pattern: "-release$"
value: << pipeline.git.branch >>
Expand All @@ -45,7 +45,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'zachw/fix-vite-hoisting-issue', << pipeline.git.branch >> ]
- equal: [ 'ryanm/fix/improve-binary-cleanup-to-look-at-binary-rather-than-monorepo', << pipeline.git.branch >> ]
- matches:
pattern: "-release$"
value: << pipeline.git.branch >>
Expand All @@ -63,7 +63,7 @@ windowsWorkflowFilters: &windows-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'zachw/fix-vite-hoisting-issue', << pipeline.git.branch >> ]
- equal: [ 'ryanm/fix/improve-binary-cleanup-to-look-at-binary-rather-than-monorepo', << pipeline.git.branch >> ]
- matches:
pattern: "-release$"
value: << pipeline.git.branch >>
Expand Down Expand Up @@ -130,7 +130,7 @@ commands:
- run:
name: Check current branch to persist artifacts
command: |
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "zachw/fix-vite-hoisting-issue" ]]; then
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "ryanm/fix/improve-binary-cleanup-to-look-at-binary-rather-than-monorepo" ]]; then
echo "Not uploading artifacts or posting install comment for this branch."
circleci-agent step halt
fi
Expand Down
2 changes: 1 addition & 1 deletion npm/webpack-preprocessor/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ const preprocessor: WebpackPreprocessor = (options: PreprocessorOptions = {}): F
webpackOptions.module.rules.unshift({
test: /\.(js|ts|jsx|tsx)$/,
use: [{
loader: path.join(__dirname, 'lib/cross-origin-callback-loader'),
loader: require.resolve('@cypress/webpack-preprocessor/dist/lib/cross-origin-callback-loader.js'),
Copy link
Member

Choose a reason for hiding this comment

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

If we need to do require.resolve() within the NPM module, is there risk that custom webpack configuration will be problematic or is this only for snapshot resolution in the binary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only for snapshot resolution in the binary.

Copy link
Member

Choose a reason for hiding this comment

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

& vite doesn't have issues because we don't ship with the binary? https://github.com/cypress-io/cypress/blob/develop/npm/vite-dev-server/src/resolveConfig.ts#L14

options: {
commands: callbackReplacementCommands,
},
Expand Down
1 change: 1 addition & 0 deletions npm/webpack-preprocessor/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"watch": "rimraf dist && tsc --watch"
},
"dependencies": {
"@babel/parser": "7.13.0",
Copy link
Member

Choose a reason for hiding this comment

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

why does this need @babel/parser?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/cypress-io/cypress/blob/ryanm/fix/improve-binary-cleanup-to-look-at-binary-rather-than-monorepo/npm/webpack-preprocessor/lib/cross-origin-callback-loader.ts#L2

"bluebird": "3.7.1",
"debug": "^4.3.2",
"fs-extra": "^10.1.0",
Expand Down
51 changes: 28 additions & 23 deletions scripts/binary/binary-cleanup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,22 +36,23 @@ async function removeEmptyDirectories (directory) {
}
}

const getDependencyPathsToKeep = async () => {
const getDependencyPathsToKeep = async (buildAppDir) => {
const unixBuildAppDir = buildAppDir.split(path.sep).join(path.posix.sep)
const startingEntryPoints = [
'packages/server/index.js',
'packages/server/hook-require.js',
'packages/server/lib/plugins/child/require_async_child.js',
'packages/server/lib/plugins/child/register_ts_node.js',
'packages/rewriter/lib/threads/worker.js',
'node_modules/webpack/lib/webpack.js',
'node_modules/webpack-dev-server/lib/Server.js',
'node_modules/html-webpack-plugin-4/index.js',
'node_modules/html-webpack-plugin-5/index.js',
'node_modules/mocha-7.0.1/index.js',
]

let entryPoints = new Set([
// This is the entry point for the server bundle. It will not have access to the snapshot yet. It needs to be kept in the binary
require.resolve('@packages/server/index.js'),
// This is a dynamic import that is used to load the snapshot require logic. It will not have access to the snapshot yet. It needs to be kept in the binary
require.resolve('@packages/server/hook-require.js'),
// These dependencies are started in a new process or thread and will not have access to the snapshot. They need to be kept in the binary
require.resolve('@packages/server/lib/plugins/child/require_async_child.js'),
require.resolve('@packages/server/lib/plugins/child/register_ts_node.js'),
require.resolve('@packages/rewriter/lib/threads/worker.ts'),
// These dependencies use the `require.resolve(<dependency>, { paths: [<path>] })` pattern where <path> is a path within the cypress monorepo. These will not be
// pulled in by esbuild but still need to be kept in the binary.
require.resolve('webpack'),
require.resolve('webpack-dev-server', { paths: [path.join(__dirname, '..', '..', 'npm', 'webpack-dev-server')] }),
require.resolve('html-webpack-plugin-4', { paths: [path.join(__dirname, '..', '..', 'npm', 'webpack-dev-server')] }),
require.resolve('html-webpack-plugin-5', { paths: [path.join(__dirname, '..', '..', 'npm', 'webpack-dev-server')] }),
...startingEntryPoints.map((entryPoint) => path.join(unixBuildAppDir, entryPoint)),
// These dependencies are completely dynamic using the pattern `require(`./${name}`)` and will not be pulled in by esbuild but still need to be kept in the binary.
...['ibmi',
'sunos',
Expand All @@ -61,7 +62,7 @@ const getDependencyPathsToKeep = async () => {
'linux',
'openbsd',
'sunos',
'win32'].map((platform) => require.resolve(`default-gateway/${platform}`)),
'win32'].map((platform) => path.join(unixBuildAppDir, `node_modules/default-gateway/${platform}.js`)),
])
let esbuildResult
let newEntryPointsFound = true
Expand All @@ -77,8 +78,10 @@ const getDependencyPathsToKeep = async () => {
outdir: workingDir,
platform: 'node',
metafile: true,
absWorkingDir: unixBuildAppDir,
external: [
'./packages/server/server-entry',
'./transpile-ts',
'./server-entry',
'fsevents',
'pnpapi',
'@swc/core',
Expand All @@ -95,9 +98,9 @@ const getDependencyPathsToKeep = async () => {
let entryPoint

if (warningSubject.startsWith('.')) {
entryPoint = path.join(__dirname, '..', '..', path.dirname(warning.location.file), warningSubject)
entryPoint = path.join(unixBuildAppDir, path.dirname(warning.location.file), warningSubject)
} else {
entryPoint = require.resolve(warningSubject)
entryPoint = require.resolve(warningSubject, { paths: [path.join(unixBuildAppDir, path.dirname(warning.location.file))] })
}

if (path.extname(entryPoint) !== '' && !entryPoints.has(entryPoint)) {
Expand All @@ -114,16 +117,18 @@ const getDependencyPathsToKeep = async () => {
const cleanup = async (buildAppDir) => {
// 1. Retrieve all dependencies that still need to be kept in the binary. In theory, we could use the bundles generated here as single files within the binary,
// but for now, we just track on the dependencies that get pulled in
const keptDependencies = [...await getDependencyPathsToKeep(), 'package.json', 'packages/server/server-entry.js']
const keptDependencies = [...await getDependencyPathsToKeep(buildAppDir), 'package.json', 'packages/server/server-entry.js']

// 2. Gather the dependencies that could potentially be removed from the binary due to being in the snapshot
const potentiallyRemovedDependencies = [...snapshotMetadata.healthy, ...snapshotMetadata.deferred, ...snapshotMetadata.norewrite]

// 3. Remove all dependencies that are in the snapshot but not in the list of kept dependencies from the binary
await Promise.all(potentiallyRemovedDependencies.map(async (dependency) => {
// marionette-client requires all of its dependencies in a very non-standard dynamic way. We will keep anything in marionette-client
if (!keptDependencies.includes(dependency.slice(2)) && !dependency.includes('marionette-client')) {
await fs.remove(path.join(buildAppDir, dependency.replace(/.ts$/, '.js')))
const typeScriptlessDependency = dependency.replace(/\.ts$/, '.js')

// marionette-client and babel/runtime require all of their dependencies in a very non-standard dynamic way. We will keep anything in marionette-client and babel/runtime
if (!keptDependencies.includes(typeScriptlessDependency.slice(2)) && !typeScriptlessDependency.includes('marionette-client') && !typeScriptlessDependency.includes('@babel/runtime')) {
await fs.remove(path.join(buildAppDir, typeScriptlessDependency))
}
}))

Expand Down