Convert to a hybrid (CJS + ESM) package

[hamishfagg]

As per discussions with @AndrewFarley this is a proof of concept of converting the runner to ESM.
Tests are failing with ERR_MODULE_NOT_FOUND but TS is not my forte so not sure what's going on here.

It's worth noting that there is a path to supporting both commonJS and ESM: https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

[mishushakov]

Thanks for your pull request. I will review during the weekend 😉

[mishushakov]

Sorry for waiting, I will comment on Friday 🙏

[mishushakov]

So, dear, @hamishfagg I got the runner working as ESM edition based on your PR!
Here's what I've done:

• Removed the co2.js dependency, as it needs typescript type declarations
• To fix ERR_MODULE_NOT_FOUND in tests/test.ts you have to change the import path from ../src/index to ../src/index.js
• Added some polyfill methods (below)

import { fileURLToPath } from 'url'
import path from 'node:path'

const __filename = fileURLToPath(import.meta.url)
const __dirname = path.dirname(__filename)

Did you get co2.js working? Other than that, I'd be happy to merge

[hamishfagg]

I got co2 working but I needed to add a ts declaration first
(maybe you can help out with this PR, I'm not sure if anything else is required?)

If this PR is merged won't that mean that stepci itself needs to be converted to ESM as well?

[mishushakov]

Yes, I suppose then we will have to convert anything into ESM (Cannot use "module" outside of "module" error will occur otherwise). Meanwhile, cannot you just compile got as CommonJS?

Try to change module in got's compiler options in tsconfig.json to Node16 or NodeNext

https://github.com/sindresorhus/got/blob/0ca0b7f7134f41b45a51370154041cc97c28ca60/tsconfig.json#L3-L11

[hamishfagg]

We could, though I suspect that compiling got will be more difficult than just compiling the stepCI runner now that we've figured that out :) (we only use the runner).

So is the easiest solution perhaps making the runner support commonJS and ESM like I mentioned?

[mishushakov]

Hey,
Sorry for my late reply

ESM is not a priority currently, but rather "nice to have"

I don't think compiling got as CommonJS is going to be difficult, unless they rely on many other ESM packages. I will do an experiment during the weekend and give you an update about how it went

My issue with ESM is that you can use CommonJS modules in an ESM module, but not the other way around
This would mean, that people will have to change their project to ESM modules in order to use our packages, which is not optimal for us

Another way may be to bundle all the dependencies. With this, we would have a full control of the output format. I'm not sure however, whether an NPM package can export both ESM and CommonJS entry points

Anyways,
Talk to you soon and sorry for the wait 🙏

[hamishfagg]

I understand, I'm going to try to get the runner to support both - so that it can be used by stepCI, and also by our project.

Added some polyfill methods (below)

import { fileURLToPath } from 'url'
import path from 'node:path'

const __filename = fileURLToPath(import.meta.url)
const __dirname = path.dirname(__filename)

Which file did you add this section to?

Thanks

[mishushakov]

src/index.ts

[hamishfagg]

Take a look at the changes now, I've managed to get the stepCI runner to compile as both commonJS and ESM at the same time - following the link I posted above. The ESM version uses got-ssrf to stop SSRF attacks which was the purpose of these changes.

I wasn't able to add your polyfil methods above as they seem to be esm-specific in part. But we can solve that if you like this approach.

I'm able to use this code to run our project as both commonJS and esm without any issues.

[mishushakov]

Will check out on Friday, thanks! Hopefully that have solved the issue for you 😄

[mishushakov]

So, I've decided the best path forward would be to move to ESM modules and then use a module bundler to compile it to both ESM and CommonJS. Otherwise, I could compile got as CommonJS, but this would mean the releases would be a bit behind. I will close the pull request for now and let you know when we have something we can share 😁

Thanks a lot for your input on that one, I really appreciate that 🙏

[hamishfagg]

I've decided the best path forward would be to move to ESM modules and then use a module bundler to compile it to both ESM and CommonJS

Isn't this what I've already done, other than using a separate bundler program to compile? My changes compile the runner as both ESM and CommonJS, stores both in separate folders under dist/ and allows both to be pushed to npm so that the same package can be used by both js types. Unless I'm missing something?

[mishushakov]

Hey,
Apologies if there was some misunderstanding in closing the pull request

Unfortunately the issue is more nuanced than that. TypeScript compiler only converts (transpiles) your TypeScript code to JavaScript, however this has no impact on third-party dependencies, such as the got package. To get around that, you’ll have to use a module bundler, such as swc/esbuild or other, which can process the source code of all the dependencies and convert them to something else, in our case ESM > CommonJS

The work needed to complete the ESM transition is beyond the scope of this pull request

Our best path forward would be to bundle got package and its dependencies as a CommonJS module. We'll publish the results on Friday

Hopefully, you could get your issue resolved already and I'll update you here on Friday 🙏

[mishushakov]

Here's how you can convert got to CommonJS using esbuild

1. Create a package.json with following contents:

{
  "devDependencies": {
    "esbuild": "^0.17.12",
    "got": "^12.6.0"
  },
  "scripts": {
    "build": "esbuild got/source/index.ts --bundle --platform=node --outfile=dist/index.js"
  }
}

2. Run npm install
3. Run npm run build
4. See the results in the dist folder

The only downside is that type-declarations are not bundled in the output

[hamishfagg]

I don't really understand your insistence on converting got to CommonJS.
It's an easy task to convert this runner to ESM, and you don't need to include any other tools to make your own version of a dependency - that sounds fraught with danger and later problems to me.

The ONLY issue with converting this lib to ESM is that some projects use the commonJS version - namely the main stepci project. And that problem is fixed by providing the current commonJS version of the runner (which still uses old got) alongside the ESM version. Once that's done, you don't need to convert anything else.

That's what I've already done, so we don't need to touch got or any other dependency. So I don't understand why you're talking about how I need a bundler to convert got.

[mishushakov]

The problem is that you cannot require ESM modules from a cjs package

I have done an experiment:

1. Added test.js in dist/cjs with the following contents:

const { run } = require('./index.js')

2. In my package.json updated got to latest the version:

"got": "^12.6.0",

Here's the error I'm getting:

/Users/mish/Downloads/runner/dist/cjs/index.js:39
const got_1 = __importDefault(require("got"));
                              ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/mish/Downloads/runner/node_modules/got/dist/source/index.js from /Users/mish/Downloads/runner/dist/cjs/index.js not supported.
Instead change the require of /Users/mish/Downloads/runner/node_modules/got/dist/source/index.js in /Users/mish/Downloads/runner/dist/cjs/index.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/mish/Downloads/runner/dist/cjs/index.js:39:31)
    at Object.<anonymous> (/Users/mish/Downloads/runner/dist/cjs/test.js:1:17) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v18.13.0

I will reopen in case you want to pull a more solid fix

[hamishfagg]

Yes, you can't import the new versions of got into the runner. That is the problem that this PR solves.

I'm going to go back to the start because I don't feel like I'm being understood.

What we have now:

stepci [commonJS] -> stepci/runner [commonJS] -> got v11.x [commonJS]
Our app [ESM]     -> stepci/runner [commonJS] -> got v11.x [commonJS]

Everything is fine here and it works great. However...

The problem

We would like to use features from got v12.x. And if we do this:

Our app [ESM] -> stepci/runner [commonJS] -> got v12.x [ESM]

Then we run into the problem that you described. You can't import ESM from CommonJS.
And thinking more long-term, it seems like people are shifting towards ESM in general and this will only get more common.

Solution 1

We could convert all of your projects to ESM and upgrade to got v12.x. This would solve the problem:

stepci [ESM]  -> stepci/runner [ESM] -> got v12.x [ESM]
Our app [ESM] -> stepci/runner [ESM] -> got v12.x [ESM]

But as you have pointed out, this is labour intensive for stepci (and I agree it's too hard).

Solution 2

You are suggesting this solution:

stepci [commonJS] -> stepci/runner [commonJS] -> got v12.x [commonJS]
Our app [ESM]     -> stepci/runner [commonJS] -> got v12.x [commonJS]

Imo this is just kicking the can down the road, as any changes to got might break your bundler or cause vulnerabilities that we would be unaware of.

Solution 3

THIS IS WHAT THIS PR IMPLEMENTS
Compile the runner as both ESM and CommonJS. This provides support for both got v11.x and v12.x, and also provides an gentle upgrade path/sunset period for got v11.x in stepci. With this solution we end up with this:

stepci [commonJS] -> stepci/runner [commonJS] -> got v11.x [commonJS]
Our app [ESM]     -> stepci/runner [ESM]      -> got v12.x [ESM]

Both versions of the runner can be distributed via npm simultaneously, so that the correct version for any project is used automatically (this will require zero changes for anyone importing the runner into their project). This 'hybrid' type of package is becoming common because of the mess with commonJS/ESM, so it is not something special.

Please take a look at the changes in this PR and see that it compiles a ESM version of the runner as well as a commonJS version, and configures them to be used based on the import type used.

[mishushakov]

Regarding Solution 3:
How can you include 2 versions of got simultaneously?

[hamishfagg]

You don't, you build the runner twice - one with got 11 and one with got 12.

This link I provided at the very start of this PR explains all of this, it is not something I've invented: https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

[mishushakov]

Doesn't make sense to me. Have you tried my example above?

The runner is a package. How can a package have "got" as a dependency in package.json with 2 different versions 11 and 12?

You do realize, that transpiling from TS > JS does not bundle the packages? They're still downloaded from package.json

IMO the cleanest way to resolve the issue would be to convert to ESM. But we don't have any plans to do that currently and this comes with its own downsides

[hamishfagg]

Have you tried my example above?

Yes. Have you tried this PR?
Make a commonJS package and a ESM package that import this PR with import and require. They will both work. Use my add_dist_files branch if you want an already-built version.

How can a package have "got" as a dependency in package.json with 2 different versions

It doesn't. It is built twice. Please read the contents of this PR.
(in this instance the ESM module gets got from got-ssrf, which requires got v12. But it is very easy to use a different version of got directly)

You do realize... They're still downloaded from package.json

Yes. Please read the contents of package.json in this PR:

  "main": "dist/cjs/index.js",
  "module": "dist/esm/index.js",
  "exports": {
    ".": {
      "import": "./dist/esm/index.js",
      "require": "./dist/cjs/index.js"
    }
  }

Depending on how runner is imported (require or import), it will grab the cjs or esm version of the package.

Again, this is not something new and it is used all over the place. The co2 package in runner uses the exact same technique:
https://github.com/thegreenwebfoundation/co2.js/blob/15d4b2d397999c9e40915ad7fcc0aaa8234c244c/package.json#L5-L12

[mishushakov]

Thanks for the explanation!
I know you can export both cjs and esm.

My question was actually how you could have two version of got in a package.json as a dependency, simultaneously.
Now I see that you're actually using two different packages: got and got-ssrf and overwrite the import in the generated .js accordingly.

That's suboptimal for us, as we want to depend on the latest version of got in both cases and this can only be solved by bundling got in case of cjs.

So the issue here is really: we want both cjs and esm export, got v12 only works with esm, got v11 works on both

What I'm proposing:

1. Make the runner package compile to esm
2. Upgrade got dependency to version 12
3. Additionally, bundle the runner and its dependencies as a cjs package, so you could use it both in esm and cjs environments

Does this sound good to you?

Ps. Sorry I was getting heated in my previous responses 😄

[hamishfagg]

Ps. Sorry I was getting heated in my previous responses 😄

Likewise :)

What I'm proposing:..
3. Additionally, bundle the runner and its dependencies as a cjs package, so you could use it both in esm and cjs environments

So in this step 3, got v12 would be transpiled/converted to CJS? Just so I understand right

There's also the possibility of 'dynamic importing' got v12 in CJS, but TBH I don't understand enough about js to know if that's a viable solution: https://github.com/sindresorhus/got#install