-
Notifications
You must be signed in to change notification settings - Fork 0
Convert code base to ESM #91
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: master
Are you sure you want to change the base?
Conversation
| gulp.registry(ForwardReference()) | ||
|
|
||
| // define default config | ||
| let defaults = { |
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.
Config was moved to new, separate file lib/config.js so that it can now be imported where required, instead of passing it around function parameters.
|
Note this PR is affected by this: gulpjs/gulp-cli#269 As such, it only works on Node <= v22.11.0 |
Co-authored-by: Copilot <[email protected]>
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 converts the entire codebase from CommonJS to ES Module (ESM) format, updating import/export syntax, task registration patterns, and modernizing several dependencies. The changes enable the project to use modern JavaScript module syntax while maintaining all existing functionality.
- Convert all
require()statements toimportstatements with proper Node.js module prefixes - Replace
module.exportswith namedexportstatements throughout the codebase - Update Gulp task registration from deprecated
gulp.task()to direct function exports withdisplayNameproperties
Reviewed Changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Add "type": "module" and update dependencies to ESM-compatible versions |
| gulpfile.js | Convert to ESM, remove config loading logic, add task exports from all task modules |
| lib/sake.js | Convert to ESM, move config logic to separate module, update import syntax |
| lib/config.js | New module extracted from gulpfile.js containing configuration loading logic |
| tasks/*.js | Convert all task files to ESM with named exports and updated import statements |
| pipes/*.js | Convert pipe utility modules to ESM with named exports |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| }, function (error) { | ||
| done(error) | ||
| async.eachLimit(milestones, 5, function (milestone, cb) { | ||
| let description = codename().generate(['unique', 'alliterative', 'random'], ['adjectives', 'animals']).join(' ') |
Copilot
AI
Aug 22, 2025
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.
The codename usage has been corrected. Since codename is imported as a default export, it should be called as codename() rather than codename.generate().
| let description = codename().generate(['unique', 'alliterative', 'random'], ['adjectives', 'animals']).join(' ') | |
| let description = codename(['unique', 'alliterative', 'random'], ['adjectives', 'animals']).join(' ') |
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 tested this in isolation and confirmed it does work.
| // Compile all plugin assets | ||
| const compile = (done) => { | ||
| // default compile tasks | ||
| let tasks = [lintPhpTask, 'scripts', stylesTask, minifyImagesTask] // NOTE: do not import the `scripts` constant here, otherwise it creates a circular dependency |
Copilot
AI
Aug 22, 2025
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.
The hardcoded string 'scripts' in the tasks array creates an inconsistency with other imported tasks and may cause issues since task registration has changed to function exports. Consider resolving the circular dependency or using a different approach.
| let tasks = [lintPhpTask, 'scripts', stylesTask, minifyImagesTask] // NOTE: do not import the `scripts` constant here, otherwise it creates a circular dependency | |
| let tasks = [lintPhpTask, compileScripts, stylesTask, minifyImagesTask] // Use function reference instead of string for scripts task |
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.
Suggested inclusion of compileScripts is wrong. It's actually referencing the scriptsTask from scripts.js.
The reason we can't import it is because compile.js would be importing a function from scripts.js. But scripts.js already imports from compile.js and referencing that again would trigger compile.js to re-import from scripts.js -- hence kind of an infinite loop there.
I ran it by Claude and Claude agreed current implementation is best:
Prompt
In tasks/compile.js line 155 I note a circular dependency. The 'scripts' string references the
scriptsTaskdefined in tasks/scripts.js . The problem was that if I import the scriptsTask function, it comes from a file that also then imports stuff from compile.js, which would then import from scripts.js again, thus a circular dependency.
How would you fix this, or is the way i have it now best?
Do not actually change the code, just describe a plan for me, or if what I have now is already the best/simplest option then say that.
Response
I can see the circular dependency issue clearly:
- tasks/compile.js line 3 imports compileScripts from tasks/scripts.js
- tasks/scripts.js line 3 imports compileScripts from tasks/compile.js
- tasks/compile.js line 155 uses the string 'scripts' instead of importing scriptsTask to avoid the circular dependency
Your current approach using the string 'scripts' is actually the best and simplest solution for this specific case. Here's why:
Why your current approach is optimal:
- Gulp can resolve task names as strings at runtime, avoiding the circular import
- It's the standard Gulp pattern for referencing tasks by name
- Clean and simple - no architectural changes needed
- Follows the principle of least change
Alternative approaches would be more complex:
- Dependency injection - Pass tasks as parameters, but adds complexity
- Shared module - Move common tasks to a separate file, but requires refactoring
- Lazy imports - Use dynamic imports, but adds async complexity
- Task registry - Create a central registry, but overengineers the solution
Your current solution is exactly what Gulp was designed for - referencing tasks by their registered names rather than direct function imports when circular dependencies would occur.
Co-authored-by: Copilot <[email protected]>
Issue: https://godaddy-corp.atlassian.net/browse/MWC-17961
Summary
This PR converts the codebase to ESM. The changes/impacts are:
Require => import
Instead of
const { spawn } = require('child_process')we'd use this syntax:import { spawn } from 'node:child_process'So moving from
requiretoimportExports
module.exportsis no longer a thing, so all instances of that have been changed.Task registration
Although not strictly part of the ESM migration, I also updated our deprecated usage of the gulp
task()function. See https://gulpjs.com/docs/en/api/taskPreviously we registered a task using
task('my-task-name', functionNameHere. This is deprecated and the new way to do it is to simply export the function -- e.g.export function functionNameHereOne challenge was how to get our old task names back. Previously we could just specify the task name as a string, which allowed us to use characters like:
copy:build. But we don't have that same flexibility when exporting a function directly, since a function can't contain some special characters. The way around that is to set thedisplayNameon the thing we're exporting. Example:Without the change, we'd have to run
sake validateReadmeHeaders, but by setting the display name,sake validate:readme_headersstill works!Task Updates
I tested each of these as I went.
build.jsbump.jsbundle.jsclean.jscompile.jsconfig.jscopy.jsdefacceinate.jsdeploy.jsgithub.jsimagemin.jslint.jsmakepot.jsprerelease.jsprompt.jsscripts.jsshell.jsstyles.jsupfw.jsvalidate.jswatch.jswc.jszip.jsQA
Build / zip
sake zipusing a plugin that includes blocks (like a payment gateway)Deployment
To be completed only once the above has been done successfully!
sake deploysake deploysake deployAfter merge