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

feat: add mjs support #1786

Closed
wants to merge 13 commits into from
Closed

feat: add mjs support #1786

wants to merge 13 commits into from

Conversation

rishabh3112
Copy link
Member

What kind of change does this PR introduce?
Support for .mjs configs

Did you add tests for your changes?

If relevant, did you update the documentation?

Summary
It is extension of @anshumanv's PR with support for esm configs using esm package.

Does this PR introduce a breaking change?

Other information
The standard await import() will not work as nodejs/node#32985 has been reverted.
Using esm package in this PR to have esm interop

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Tests are failing

@rishabh3112
Copy link
Member Author

Tests are failing

@evenstensberg Yes, because esm package is not working properly here. I couldn't find upon investigation on how to make it work.
So, I need help.

@alexander-akait
Copy link
Member

I think we don't need package, we can use this idea https://github.com/gulpjs/gulp-cli/blob/master/lib/shared/require-or-import.js

@webpack-bot
Copy link

@rishabh3112 Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@evenstensberg Please review the new changes.

@rishabh3112
Copy link
Member Author

I think we don't need package, we can use this idea https://github.com/gulpjs/gulp-cli/blob/master/lib/shared/require-or-import.js

Same not working again locally.

@alexander-akait
Copy link
Member

@rishabh3112 what is node you are use (version)?

@rishabh3112
Copy link
Member Author

Tried on 12, 13 and 14. (I use fnm)

@alexander-akait
Copy link
Member

Maybe something broken in fnm? I use nvm and tests for gulp-cli worked with mjs

@rishabh3112
Copy link
Member Author

Maybe something broken in fnm? .

Can't say for sure. Lets see CI, if still not passed then can you try checking out this branch and testing locally once?

@anshumanv
Copy link
Member

I tried with nvm in the past, no luck 😞

@anshumanv
Copy link
Member

Yep would be great if @evilebottnawi can check once

@rishabh3112 rishabh3112 closed this Dec 3, 2020
@rishabh3112 rishabh3112 deleted the feat/mjs-support branch December 3, 2020 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants