-
Notifications
You must be signed in to change notification settings - Fork 231
chore: enhance npm start command to run compass sync and sandbox COMPASS-9851 #7338
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
base: main
Are you sure you want to change the base?
Conversation
child_process.execFileSync('pnpm', ['run', 'init'], { | ||
cwd: process.env.MMS_HOME, | ||
stdio: 'inherit', | ||
}); |
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.
You don't need init, it's doing the same thing that start will do right after
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.
Yea I always thought the output looks similar but pressing enter on the command in my terminal history was easier than looking into why 🤦🏻 lol thank you!
desktop: { enabled: false, args: [] as string[] }, | ||
sandbox: { enabled: false, args: [] as string[] }, |
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.
desktop and sandbox currently have ports in common, so you can't run then in parallel, you might want to adjust that
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.
just made sandbox something you have to run alone because of the way things are shared, I think desktop & sync are more likely to be used in parallel? at least by our team. But happy to dig deeper if you have a better idea of what needs to be changed.
Like I changed the compass-web dev server port but then something else is using that config because I get a port reuse error 🤔
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.
Yeah, this makes sense
|
||
const paddedName = targetName.padEnd(8); | ||
console.log(`start | ${spawnArgs[0]} ${spawnArgs[1].join(' ')}`); | ||
const subProcess = child_process.spawn(...spawnArgs); |
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.
You need to pipe stdin also. Desktop command in particular has support for extra commands to restart the process / pass extra args:
desktop | <i> [webpack-plugin-start-electron] Starting electron application
desktop | <i> [webpack-plugin-start-electron] - Ctrl+R to restart the main process
desktop | <i> [webpack-plugin-start-electron] - Ctrl+A to restart the main process with extra arguments
And none of it is working at the moment
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.
should be working now 🎉
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.
Pull Request Overview
This PR adds a new npm start
command that allows launching multiple Compass environments (desktop, sandbox, sync) in parallel to streamline testing across different platforms.
- New multi-process launcher script that can start desktop, sandbox, and sync environments together
- Enhanced sync script converted from JS to MJS with improved MMS dev server management
- Updated package.json configurations to support the new start command
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
scripts/start.mts | New multi-process launcher script handling argument parsing and subprocess management |
scripts/start.md | Help documentation for the new start command |
packages/compass-web/scripts/sync-dist-to-mms.mjs | Converted sync script from JS to MJS with enhanced MMS server handling |
packages/compass-web/scripts/sync-dist-to-mms.js | Removed old JS version of sync script |
packages/compass-web/package.json | Updated sync command to use new MJS script |
packages/compass-web/.eslintrc.js | Added ESLint configuration for MJS files |
package.json | Updated root start command to use new launcher script |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
366558b
to
8068f7b
Compare
// If a copy is already in progress, return early (debounce) | ||
if (oneSec) return; | ||
fs.cpSync(srcDir, destDir, { recursive: true }); | ||
oneSec = timers.setTimeout(1000); |
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.
I might be misreading the logic here, but I think we should rather have a trailing debounce, not a leading one, or both (a good thing about lodash was that it was doing both: first run is immediate, if there was a second one during the timeout, it will also happen when debounce is ready to resolve)
packages/compass-web/.eslintrc.js
Outdated
tsconfigRootDir: __dirname, | ||
project: ['./tsconfig.json'], | ||
}, | ||
overrides: [ |
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.
I think we might want to extens our shared eslint config with this, not only compass web, but not a blocker
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.
I agree but this problem also goes away if we use .mts which is readily accessible now so we can use that until we're forced to use mjs for some reason
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.
Looks good! Few small notes, nothing blocking
scripts/start.mts
Outdated
} | ||
|
||
// Wait a shorter time for graceful shutdown | ||
await timers.setTimeout(3000); |
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.
Small nit / note: I think might make sense to bump it a little, to at least 10s maybe. If this is specifically for desktop webpack build, you kinda do want it to exit gracefully to avoid corrupting the build cache as this has an effect on the next start up time
.filter(Boolean) | ||
.join(' '); | ||
|
||
devServer = child_process.spawn( |
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.
If we're not adding some sort of explicit way to disable strict engine check here, would be nice to monitor for the error and print something along the lines that the check can be disabled with NPM_CONFIG_ENGINE_STRICT=false
flag
Description
npm start++
There is now a script responsible for multi-process launching to enable and encourage mulitplatform testing of compass and DE changes.
npm start
will still launch compass as per usual.New Commands:
npm start desktop sync sandbox
Provide each one of the subcommands optionally to combine launching each environment to start testing changes in parallel.
Note that the sync command still requires use of a redirector / redwood extension in your browser.
Checklist
Motivation and Context
In the effort of DE and Compass unification hopefully this change will make verifying in both environements easy and commonplace to squash inconsistency early on in development.
Open Questions
Dependents
Types of changes