Skip to content

fix: remove extra bundled electron in 10.0 binary #20932

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

Merged
merged 5 commits into from
Apr 6, 2022
Merged

fix: remove extra bundled electron in 10.0 binary #20932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
66edbe0
fix: remove duplicate electron in binary dependencies
tgriesser Apr 5, 2022
95e6c8d
move openProject
tgriesser Apr 6, 2022
4d72fac
fix test failure
tgriesser Apr 6, 2022
8446c36
Merge branch '10.0-release' into tgriesser/fix/UNIFY-1388-take-2
tgriesser Apr 6, 2022
72d06c0
Decrease the MAX_ALLOWED_SIZE_MB
tgriesser Apr 6, 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
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@
"dedent": "^0.7.0",
"del": "3.0.0",
"detect-port": "^1.3.0",
"electron": "15.3.5",
"electron-builder": "^22.13.1",
"electron-notarize": "^1.1.1",
"enzyme-adapter-react-16": "1.12.1",
Expand Down
1 change: 0 additions & 1 deletion packages/data-context/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
"dayjs": "^1.9.3",
"dedent": "^0.7.0",
"ejs": "^3.1.6",
"electron": "14.1.0",
Copy link
Contributor

@lmiller1990 lmiller1990 Apr 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should we not leave it as a devDependency here, since it's imported in data-context? (if only for types with import type)? Or is this the same thing as what @flotwig mentioned above, where we now rely on root package.json to have the dependency via hoisting?

Copy link
Member Author

@tgriesser tgriesser Apr 6, 2022

Choose a reason for hiding this comment

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

Yeah it's possible if we have it defined in multiple places as devDependencies someone will update & forget to update in all places and have an extra electron installed locally. For packages like this that need to be enforced as singletons, I find it's simples to keep it explicitly defined in a single place at the root.

Copy link
Contributor

@lmiller1990 lmiller1990 Sep 1, 2022

Choose a reason for hiding this comment

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

I think this might have broke the ELECTRON_RUN_AS_NODE feature. docs: https://docs.cypress.io/guides/continuous-integration/introduction#Running-headless-tests-without-Xvfb

#23636

"endent": "2.0.1",
"execa": "1.0.0",
"front-matter": "^4.0.2",
Expand Down
4 changes: 2 additions & 2 deletions packages/electron/lib/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
const _ = require('lodash')
const os = require('os')
const path = require('path')
const pkg = require('../package.json')
const pkg = require('../../../package.json')
const paths = require('./paths')
const log = require('debug')('cypress:electron')
const fs = require('fs-extra')
Expand All @@ -12,7 +12,7 @@ let electronVersion

// ensure we have an electronVersion set in package.json
if (!(electronVersion = pkg.devDependencies.electron)) {
throw new Error('Missing \'electron\' devDependency in ./package.json')
throw new Error('Missing \'electron\' devDependency in root package.json')
}

module.exports = {
Expand Down
1 change: 0 additions & 1 deletion packages/electron/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
"minimist": "1.2.6"
},
"devDependencies": {
"electron": "15.3.5",
Copy link
Contributor

@flotwig flotwig Apr 6, 2022

Choose a reason for hiding this comment

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

we are back to relying on the magic of hoisting to make our dependencies resolve 😢

Copy link
Member Author

@tgriesser tgriesser Apr 6, 2022

Choose a reason for hiding this comment

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

I noted this in the PR comment:

Moves electron to the binary root, since it is depended on by multiple modules. It's technically a "virtual" dependency, in the sense that it doesn't get bundled when the app is shipped, so having it as a root devDependency seems correct.

It's not magic, it's the node module resolution. At least in this case it's explicitly hoisted at the root

Copy link
Contributor

@flotwig flotwig Apr 6, 2022

Choose a reason for hiding this comment

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

It seems more correct to me to put the electron dependency in @packages/electron since the purpose of that package is to expose it, and not put it in root because it's more convenient, have some separation of concerns. Yarn also explicitly discourages adding deps to the root workspace. This is just being done this way because it's convenient 😛

Copy link
Member Author

@tgriesser tgriesser Apr 6, 2022

Choose a reason for hiding this comment

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

Yeah I hear that. And if that's what we want then we'd prob need to scope that out a bit, b/c that'd be a decent change as it's not how importing 'electron'. has worked in the server or elsewhere (pre-dating our time at Cypress).

It's also not adding as a dep, it's a just a devDep, since require('electron') is just a builtin in the node envrionment when executing in the binary prod build.

Copy link
Contributor

@lmiller1990 lmiller1990 Apr 6, 2022

Choose a reason for hiding this comment

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

Just looking, isn't it weird that we listed electron as a devDependency in packages/electron in the first place - since we are not actually requiring it anywhere in there?

image

Edit: I suppose it was due to

// ensure we have an electronVersion set in package.json
if (!(electronVersion = pkg.devDependencies.electron)) {
  throw new Error('Missing \'electron\' devDependency in ./package.json')
}

or the fact we have electron-package as a dependency.

I don't fully understand why it's weird to put it at the top level, considering it's a hard dependency across multiple packages. What exactly makes workplace root dependencies a bad practice? I haven't worked with complex monorepos before Cypress, so I don't fully understand the implications - asking for my own understanding, as opposed to a review of this part of the PR.

Copy link
Member Author

@tgriesser tgriesser Apr 6, 2022

Choose a reason for hiding this comment

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

I don't fully understand why it's weird to put it at the top level, considering it's a hard dependency across multiple packages.

I personally think it's fine to have it at the root, it actually avoids hoisting, where hoisting is defined as a peer packages/ dependency having a dependency, which gets installed at the root node_modules rather than packages/$pkg/node_modules for reasons that should be an implementation detail of the package manager.

By putting it in the root as a devDependency, you're basically saying "this is needed in multiple places in the repo during development, so keep a single copy here". I actually kind of wish we did that with more dev only things like mocha, chai, etc. like we do in services repo, instead we have like 20 different versions of each library that get installed everywhere during development.

Copy link
Contributor

@lmiller1990 lmiller1990 Apr 6, 2022

Choose a reason for hiding this comment

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

Yeah was thinking the same thing, we definitely need to have a centralised mocha/chai for consistency across the mono repo.

"electron-packager": "15.4.0",
"execa": "4.1.0",
"mocha": "3.5.3",
Expand Down
3 changes: 1 addition & 2 deletions packages/server/lib/cypress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ const Promise = require('bluebird')
const debug = require('debug')('cypress:server:cypress')
const { getPublicConfigKeys } = require('@packages/config')
const argsUtils = require('./util/args')
const { openProject } = require('../lib/open_project')

const warning = (code, args) => {
return require('./errors').warning(code, args)
Expand Down Expand Up @@ -118,7 +117,7 @@ module.exports = {
openProject (options) {
// this code actually starts a project
// and is spawned from nodemon
openProject.open(options.project, options)
require('../lib/open_project').open(options.project, options)
},

start (argv = []) {
Expand Down
4 changes: 2 additions & 2 deletions packages/server/lib/modes/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ const debug = require('debug')('cypress:server:run')
const Promise = require('bluebird')
const logSymbols = require('log-symbols')
const assert = require('assert')
const { getCtx } = require('@packages/data-context')

const recordMode = require('./record')
const errors = require('../errors')
Expand Down Expand Up @@ -664,7 +663,8 @@ const createAndOpenProject = async (options) => {
project: _project,
config: _config,
projectId: _projectId,
configFile: getCtx().lifecycleManager.configFile,
// Lazy require'd here, so as to not execute until we're in the electron process
configFile: require('@packages/data-context').getCtx().lifecycleManager.configFile,
}
}

Expand Down
5 changes: 4 additions & 1 deletion scripts/binary/zip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,10 @@ const checkZipSize = function (zipPath) {
const zipSize = filesize(stats.size, { round: 0 })

console.log(`zip file size ${zipSize}`)
const MAX_ALLOWED_SIZE_MB = os.platform() === 'win32' ? 295 : 300
// Before you modify these max sizes, check and see what you did that might have
// done to increase the size of the binary, and if you do need to change it,
// call it out in the PR description / comments
const MAX_ALLOWED_SIZE_MB = os.platform() === 'win32' ? 295 : 200
const MAX_ZIP_FILE_SIZE = megaBytes(MAX_ALLOWED_SIZE_MB)

if (stats.size > MAX_ZIP_FILE_SIZE) {
Expand Down
2 changes: 2 additions & 0 deletions system-tests/__snapshots__/reporters_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ Require stack:
- lib/reporter.js
- lib/project-base.ts
- lib/open_project.ts
- lib/makeDataContext.ts
- lib/modes/index.ts
- lib/cypress.js
- index.js
-
Expand Down
11 changes: 1 addition & 10 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3476,7 +3476,7 @@
resolved "https://registry.yarnpkg.com/@discoveryjs/json-ext/-/json-ext-0.5.2.tgz#8f03a22a04de437254e8ce8cc84ba39689288752"
integrity sha512-HyYEUDeIj5rRQU2Hk5HTB2uHsbRQpF70nvMhVzi+VJR0X+xNEhjPui4/kBf3VeH/wqD28PT4sVOm8qqLjBrSZg==

"@electron/get@^1.0.1", "@electron/get@^1.13.0", "@electron/get@^1.6.0":
"@electron/get@^1.13.0", "@electron/get@^1.6.0":
version "1.13.1"
resolved "https://registry.yarnpkg.com/@electron/get/-/get-1.13.1.tgz#42a0aa62fd1189638bd966e23effaebb16108368"
integrity sha512-U5vkXDZ9DwXtkPqlB45tfYnnYBN8PePp1z/XDCupnSpdrxT8/ThCv9WCwPLf9oqiSGZTkH6dx2jDUPuoXpjkcA==
Expand Down Expand Up @@ -19773,15 +19773,6 @@ electron-to-chromium@^1.3.247, electron-to-chromium@^1.3.378, electron-to-chromi
resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.4.67.tgz#699e59d6959d05f87865e12b3055bbcf492bbbee"
integrity sha512-A6a2jEPLueEDfb7kvh7/E94RKKnIb01qL+4I7RFxtajmo+G9F5Ei7HgY5PRbQ4RDrh6DGDW66P0hD5XI2nRAcg==

electron@14.1.0:
version "14.1.0"
resolved "https://registry.yarnpkg.com/electron/-/electron-14.1.0.tgz#126361b7c2a38057004f888b94a52246a502157c"
integrity sha512-MnZSITjtdrY6jM/z/qXcuJqbIvz7MbxHp9f1O93mq/vt7aTxHYgjerPSqwya/RoUjkPEm1gkz669FsRk6ZtMdQ==
dependencies:
"@electron/get" "^1.0.1"
"@types/node" "^14.6.2"
extract-zip "^1.0.3"

electron@15.3.5:
version "15.3.5"
resolved "https://registry.yarnpkg.com/electron/-/electron-15.3.5.tgz#62fc7d2289d3f47e9e05c0aa9bb6d929a6faf398"
Expand Down