Add ESM support #214
Add ESM support #214
Conversation
| var exported = require(env.configPath); | ||
| requireOrImport(env.configPath, function(err, exported) { | ||
| if (err) { | ||
| console.error(err); |
There was a problem hiding this comment.
Previously, if require() failed we got an unhandled exception on the module level. So I use console.error()+exit() here to mimick the old behavior as close as possible, or would you rather log this error differently? (I noticed that you generally seem to use gulplog).
Also I'm aware that I still have to do similar changes to the other Done.lib/versioned/*/index.js scripts. But I'd first like to get your feedback on the general approach before moving forward. What do you think?
There was a problem hiding this comment.
I think this works well! Good solution :)
| importESM = new Function('id', 'return import(id);'); | ||
| } catch (e) { | ||
| importESM = null; | ||
| } |
There was a problem hiding this comment.
Node.js <10 errors out with a SyntaxError when loading a script that uses import(). So I dynamically create a function for that and catch the SyntaxError. That way we can keep supporting all Node.js versions all the way back to 0.10.
| cjs = require(path); | ||
| } catch (e) { | ||
| if (pathToFileURL && importESM && e.code === 'ERR_REQUIRE_ESM') { | ||
| var url = pathToFileURL(path); |
There was a problem hiding this comment.
I just added this as I realized that import() fails if providing a Windows file path.
|
This is absolutely amazing! Thank you for all the work you've done here 🙇 I'm going to get this merged and add some of your comments from GitHub into the source for "future us" I believe I also need to add the |
|
Sorry this is taking so long. I didn't realize getting the |
|
I just pushed up the remaining changes that needed to land for this to work with Sorry for the delay. |
|
Running into some issues with ESM support on node 10 for Windows. Need to dig into that. |
|
Do you have more details? Tests pass on my Windows machine with Node.js 10.21.0. |
|
@snoack https://ci.appveyor.com/project/gulpjs/gulp-cli/builds/33284247 And in a different run this happened: https://ci.appveyor.com/project/gulpjs/gulp-cli/builds/33284205/job/an9h465n99lugtkw |
|
Of course I create a branch and start debugging and it starts working 🤷. |
|
FWIW, I was not able to reproduce this locally on Windows 10, not even with Node.js 10.20.1 (32-bit), the exact same version you got on Appveyor. However, a quick web search for 3221225477 (the exit status Node.js was failing with on Appveyor) indicates some kind of crash likely related to environmental issues. |
|
Thanks for digging in! It definitely seems like a weird issue with AppVeyor. Anyway, this has just been released as v2.3.0! Thanks for all your work here 🎉 |
See gulpjs/interpret#65.