Skip to content

Conversation

@mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Nov 11, 2025

Makes a pass through our top level gulpfiles to convert them to modules

Kicked off private full build to confirm these changes work as expected

Makes a pass through our top level gulpfiles to convert them to modules
Copilot AI review requested due to automatic review settings November 11, 2025 23:29
Copilot finished reviewing on behalf of mjbvz November 11, 2025 23:32
Copy link
Contributor

Copilot AI left a 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 converts the top-level gulpfiles from CommonJS to ES modules by changing file extensions to .mjs and updating import/export syntax.

  • Converts require() to import statements with proper ES module syntax
  • Updates module.exports and exports.* to export statements
  • Adds __dirname handling for ES modules using fileURLToPath(new URL('.', import.meta.url))

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

File Description
gulpfile.mjs Simplified to directly import the build gulpfile, removing the require() wrapper
build/gulpfile.mjs Converted to ES modules with proper imports and changed .forEach to .map for loading gulpfiles
build/gulpfile.*.mjs Converted all build gulpfiles to ES modules with consistent import patterns and export syntax
extensions/*/package.json Updated gulpfile references from .js to .mjs extensions in build scripts
Comments suppressed due to low confidence (3)

build/gulpfile.vscode.win32.mjs:16

  • The createRequire import is unused. This module doesn't use require() anywhere after being converted to ES modules. Remove this unused import.
    build/gulpfile.vscode.win32.mjs:24
  • import.meta.resolve() returns a Promise in Node.js and must be awaited. This will cause a runtime error when trying to use path.dirname() on a Promise object. Use createRequire(import.meta.url) with require.resolve() instead, or wrap this in an async function with await import.meta.resolve('innosetup').
    build/gulpfile.mjs:59
  • Using .map() when the return value is not used is misleading. This should remain as .forEach() since the purpose is to execute side effects (loading gulpfiles), not to transform data. The previous implementation with .forEach() was more semantically correct.

@mjbvz mjbvz marked this pull request as ready for review November 12, 2025 06:17
@vs-code-engineering vs-code-engineering bot added this to the November 2025 milestone Nov 12, 2025
@mjbvz mjbvz merged commit 0eb2840 into main Nov 12, 2025
55 checks passed
@mjbvz mjbvz deleted the dev/mjbvz/gulp-mjs branch November 12, 2025 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants