Lint scripts from the root config#3310
Merged
Merged
Conversation
Bring `scripts/` under the root `lint` target instead of leaving it outside repo lint coverage. Align the ESM scripts with the repo's `import.meta.dirname` preference and fix the pre-existing script lint issues that surfaced once `scripts/` was included. Keep CommonJS entrypoints working by limiting the `require()` exception to `scripts/**/*.js` and `scripts/**/*.cjs`. --- Generated with the help of Codex, https://openai.com Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the repository’s scripts/ directory part of the root ESLint target and updates affected scripts/config so they lint cleanly and follow the repo’s ESM path-resolution conventions.
Changes:
- Include
scripts/in the rootnpm run linttarget. - Update multiple
.mjsscripts to useimport.meta.dirnameinstead offileURLToPath(import.meta.url)/__dirname. - Add ESLint overrides for
scripts/**/*(allowrequirein CommonJS scripts; restrict__dirname/__filenameusage in ESM scripts) and fix surfaced lint issues (e.g.,hasOwnProperty, floating promises).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/start-new-ui.mjs | Switch root path resolution to import.meta.dirname. |
| scripts/prepare-dev-build-version.mjs | Reorder imports to satisfy lint ordering. |
| scripts/package-in-isolation.ts | Use safe Object.prototype.hasOwnProperty.call check. |
| scripts/package-appx.mjs | Remove local __dirname and use import.meta.dirname for paths. |
| scripts/make-dmg.mjs | Replace execSync string invocation with execFileSync + import.meta.dirname paths. |
| scripts/download-node-binary.ts | Fix lint ordering and mark async entrypoint call with void. |
| scripts/download-available-site-translations.mjs | Use import.meta.dirname for output path resolution. |
| scripts/azure-signing.cjs | Reformat env var list to satisfy lint/formatting. |
| scripts/azure-sign-hook.js | Remove per-file eslint disable now covered by ESLint override. |
| package.json | Add scripts to the root lint command target list. |
| eslint.config.mjs | Add scripts-specific overrides and ESM guardrails for __dirname/__filename. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Collaborator
📊 Performance Test ResultsComparing f2c750c vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
How AI was used in this PR
Used Codex to implement the script updates, run local verification, and surface the pre-existing script lint issues that had to be fixed once
scripts/was added to the root lint target.I reviewed the resulting diff and verification output before opening this PR.
Proposed Changes
While working with AI in this repo for the CI automation, I run into linting inconsistencies in the scripts folder a few times. This PR propose to treat the scripts as first-class linting citizens, and addresses any issue found in them.
scripts/under the rootnpm run linttarget.import.meta.dirnamepreference and add a guardrail against reintroducing__dirnameinscripts/**/*.mjs.scripts/was linted, while keeping existing CommonJS script entrypoints valid.Testing Instructions
npm run lintnpm run typecheckenv FILE_ARCHITECTURE=x64 WINDOWS_CODE_SIGNING_CERT_PASSWORD=fake node scripts/package-appx.mjsand confirm it gets past module loading and stops at the expected missing Windows SDK check.env FILE_ARCHITECTURE=x64 node scripts/make-dmg.mjsafter a full install and confirm it reachesappdmg; withnpm install --ignore-scriptsit currently stops earlier on the missing nativevolume.nodedependency.Pre-merge Checklist