-
Notifications
You must be signed in to change notification settings - Fork 247
chore(scripts): refactor update-electron to general purpose update-dependencies; replace usage in CI #7071
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
chore(scripts): refactor update-electron to general purpose update-dependencies; replace usage in CI #7071
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,11 +37,7 @@ jobs: | |
| npm install -g [email protected] | ||
|
|
||
| - name: Bump eslint | ||
| run: | | ||
| npm i --save --workspace @mongodb-js/eslint-config-compass \ | ||
| eslint@8 \ | ||
| @typescript-eslint/eslint-plugin@latest \ | ||
| @typescript-eslint/parser@latest | ||
| run: npx compass-scripts update-dependencies preset-eslint | ||
|
|
||
| - name: Create Pull Request | ||
| uses: peter-evans/create-pull-request@5e914681df9dc83aa4e4905692ca88beb2f9e91f # 7.0.5 | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| 'use strict'; | ||
| module.exports = { | ||
| electron: [ | ||
| '@electron/remote', | ||
| '@electron/rebuild', | ||
| 'browserslist', | ||
| // NB: We're always trying to update to latest major, this usually implies | ||
| // breaking changes, but those rarely affect us. If it becomes a problem, we | ||
| // can always change this code to lock it to whatever major version of | ||
| // electron compass is currently at | ||
| 'electron', | ||
| 'electron-to-chromium', | ||
| 'node-abi', | ||
| ], | ||
| eslint: [ | ||
| '@typescript-eslint/eslint-plugin', | ||
| '@typescript-eslint/parser', | ||
| 'eslint@8', // TODO: update to flat config, switch to eslint 9, remove the fixed version | ||
| 'eslint-plugin-jsx-a11y', | ||
| 'eslint-plugin-react', | ||
| 'eslint-plugin-react-hooks', | ||
| ], | ||
| // TODO(COMPASS-9443): Update update-* github actions to handle all groups as | ||
| // a matrix inside one action instead of having separate action for every | ||
| // group and add more groups following the ones in _dependabot | ||
| // mongosh: [], | ||
| // 'devtools-shared-prod': [], | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| 'use strict'; | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const { isEqual, cloneDeep } = require('lodash'); | ||
| const { | ||
| listAllPackages, | ||
| runInDir, | ||
| updatePackageJson, | ||
| findMonorepoRoot, | ||
| withProgress, | ||
| } = require('@mongodb-js/monorepo-tools'); | ||
|
|
||
| const UPDATE_CONFIGS = require('./update-dependencies-config'); | ||
|
|
||
| async function hoistSharedDependencies(newVersions) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be very clear, this flow is definitely sub optimal, we run install three (!) times, but it was also the only way we could consistently make npm do what we want instead of whatever it decides to do wrt shared dependencies and their current location in the dependency tree and debugging npm behavior on a pretty complex dependency tree is really really hard and I'm not sure it's worth it
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we decide to update other deps using this scripts, they will also be hoisted at the top. And then running locally npm bootstrap, would it create updates in lock file?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested it with a few different dependencies and lock stays stable (meaning nothing is unexpectedly unhoisted) after running script a couple of times with different deps, but to be honest I don't know how to validate that this will never happen at all, I lost any trust in npm internal algorithms at this point 😆 |
||
| try { | ||
| const root = await findMonorepoRoot(); | ||
|
|
||
| await withProgress('Cleaning up existing node_modules', async () => { | ||
| await runInDir("npx lerna exec 'rm -Rf node_modules'", root); | ||
| await runInDir('rm -Rf node_modules', root); | ||
| }); | ||
|
|
||
| const packageJsonBkp = await withProgress( | ||
| 'Updating package-lock to apply package.json changes', | ||
| async () => { | ||
| await runInDir('npm i --package-lock-only --ignore-scripts', root); | ||
| return await fs.promises.readFile(path.resolve(root, 'package.json')); | ||
| } | ||
| ); | ||
|
|
||
| await withProgress( | ||
| 'Installing new dependencies at root to make sure they are hoisted', | ||
| async () => { | ||
| const versionsToInstall = newVersions | ||
| .map((spec) => { | ||
| return spec.join('@'); | ||
| }) | ||
| .join(' '); | ||
| await runInDir( | ||
| `npm i ${versionsToInstall} --package-lock-only --ignore-scripts`, | ||
| root | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| await withProgress( | ||
| 'Cleaning-up hoisted `dependencies` from root package.json', | ||
| async () => { | ||
| await fs.promises.writeFile( | ||
| path.resolve(root, 'package.json'), | ||
| packageJsonBkp | ||
| ); | ||
| await runInDir('npm i', root); | ||
| } | ||
| ); | ||
| } catch (error) { | ||
| console.error(`Error running command: ${error}`); | ||
| } | ||
| } | ||
|
|
||
| async function getVersion(depSpec) { | ||
| const [name, versionSpec = 'latest'] = depSpec.split( | ||
| /(?<!^)@/ // split by version, avoid splitting by optional namespace | ||
| ); | ||
| const { stdout: output } = await runInDir( | ||
| `npm view ${name}@${versionSpec} version --json` | ||
| ); | ||
| const parsed = JSON.parse(output); | ||
| const version = Array.isArray(parsed) ? parsed.pop() : parsed; | ||
| return [name, version.trim()]; | ||
| } | ||
|
|
||
| async function main() { | ||
| let dependencies; | ||
|
|
||
| const args = process.argv.slice(3); | ||
|
|
||
| if (args.length === 0) { | ||
| throw new Error( | ||
| 'Missing dependencies list. Provide a list of dependencies to update or a preset name:\n\n npx compass-scripts update-dependencies <package[@version-range] [...packages] | preset>' | ||
| ); | ||
| } | ||
|
|
||
| if (args[0].startsWith('preset-')) { | ||
| const presetName = args[0].replace('preset-', ''); | ||
| dependencies = UPDATE_CONFIGS[args[0].replace('preset-', '')]; | ||
| if (!dependencies) { | ||
| throw new Error( | ||
| `Can not find update config for preset "${presetName}". Available presets: ${Object.keys( | ||
| UPDATE_CONFIGS | ||
| ).join(', ')}` | ||
| ); | ||
| } | ||
| console.log(); | ||
| console.log('Running update for preset "%s" ...', presetName); | ||
| console.log(); | ||
| } else { | ||
| dependencies = args; | ||
| } | ||
|
|
||
| const newVersions = await withProgress( | ||
| `Collection version information for packages...`, | ||
| () => { | ||
| return Promise.all( | ||
| dependencies.map((depSpec) => { | ||
| return getVersion(depSpec); | ||
| }) | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| console.log(); | ||
| console.log( | ||
| 'Updating following packages:\n\n * %s\n', | ||
| newVersions | ||
| .map((spec) => { | ||
| return spec.join('@'); | ||
| }) | ||
| .join('\n * ') | ||
| ); | ||
| console.log(); | ||
|
|
||
| const newVersionsObj = Object.fromEntries(newVersions); | ||
| let hasChanged; | ||
|
|
||
| await withProgress('Updating package.json in workspaces', async () => { | ||
| for await (const props of listAllPackages()) { | ||
| await updatePackageJson(props.location, (packageJson) => { | ||
| const origPackageJson = cloneDeep(packageJson); | ||
| for (const depType of [ | ||
| 'dependencies', | ||
| 'devDependencies', | ||
| 'peerDependencies', | ||
| 'optionalDependencies', | ||
| ]) { | ||
| if (packageJson[depType]) { | ||
| for (const packageName of Object.keys(packageJson[depType])) { | ||
| if ( | ||
| packageJson[depType][packageName] && | ||
| newVersionsObj[packageName] | ||
| ) { | ||
| packageJson[depType][ | ||
| packageName | ||
| ] = `^${newVersionsObj[packageName]}`; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| hasChanged = hasChanged || !isEqual(origPackageJson, packageJson); | ||
| return packageJson; | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| if (!hasChanged) { | ||
| console.log(); | ||
| console.log('Everything is up to date, exiting ...'); | ||
| return; | ||
| } | ||
|
|
||
| await hoistSharedDependencies(newVersions); | ||
|
|
||
| console.log(); | ||
| console.log('Successfully updated dependencies'); | ||
| } | ||
|
|
||
| main(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to update this to typescript? If it's possible that would be nice! Not a blocker.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, will need to add ts-register and allow cli.js to run typescript first, I'll take a stab as a follow-up |
||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a "breaking change" compared to what the old script was doing, manually trying this update locally it doesn't seem to break anything and as mentioned I do think we rarely affected by those, so it's probably okay to try to always update to latest, but tell me if you have thoughts on that.