Skip to content
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

Adds support for gulpfiles using ESM. #113505

Merged
merged 1 commit into from Jan 6, 2021
Merged

Adds support for gulpfiles using ESM. #113505

merged 1 commit into from Jan 6, 2021

Conversation

thsmi
Copy link
Contributor

@thsmi thsmi commented Dec 28, 2020

New Node versions support ECMAScript modules beside
the classic CommonJS modules.

This means gulp does so too. So we endup with three entry points:
The legacy gulfile.js and the new gulpfile.cjs as well as gulpfile.mjs.

When using only ESM or the ESM package, gulp suggest:
Rename 'gulpfile.js' to 'gulpfile.esm.js'. This entry point is also added.

For more details refer to https://github.com/gulpjs/gulp#use-latest-javascript-version-in-your-gulpfile

This PR fixes #

New Node versions support ECMAScript modules beside
the classic  CommonJS modules.

This means gulp does so too. So we endup with three entry points:
The legacy gulfile.js and the new gulpfile.cjs as well as gulpfile.mjs.

When using only ESM or the ESM package, gulp suggest:
Rename 'gulpfile.js' to 'gulpfile.esm.js'
@ghost
Copy link

ghost commented Dec 28, 2020

CLA assistant check
All CLA requirements met.

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@@ -70,21 +89,23 @@ function showError() {
}

async function findGulpCommand(rootPath: string): Promise<string> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I cannot deny that the refactoring here is clearer, it's easier/faster/less cognitive load to review PRs that don't include refactors and features together. The greater the number of lines changed, the less likely I am to get to it quickly (and I know I'm not alone on that :))

@alexr00 alexr00 merged commit 1228854 into microsoft:master Jan 6, 2021
@alexr00 alexr00 added this to the January 2021 milestone Jan 6, 2021
@thsmi thsmi deleted the gulp-esm-support branch January 6, 2021 16:00
@andreialecu
Copy link

andreialecu commented Jan 8, 2021

I'm using VSCode Insiders and this error started appearing a few days ago while running tasks.

Screenshot 2021-01-08 at 16 15 49

Note that I do not use gulp.

The output tab logs:

/bin/sh: gulp: command not found

Auto detecting gulp for folder ... failed with error: Error: Command failed: gulp --tasks-simple --no-color
/bin/sh: gulp: command not found

I'm not sure if this PR is relevant but wanted to report it.

cc @thsmi @alexr00

@alexr00
Copy link
Member

alexr00 commented Jan 8, 2021

Thank you @andreialecu for reporting this! Not the first time it would have been good to have some linter error for checking truthiness of a promise. Fixed with bebd066

@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants