Add cjs support

[antonmedv]

Fixes #197

[cspotcode]

At a glance, looks good. esbuild might be faster unless rollup has features that you need.

[antonmedv]

@antongolub take a look, please. What do you think?

[antongolub]

@antonmedv, I,m 3* today, so I’m a little out of shape)) May I take a look tomorrow?

[antonmedv]

Also, what about moving global pollution to zx.mjs in this version?

And keep the library index.mjs free of global extension?

[antongolub]

Definitely agree: remove global side effects from index and introduce global.mjs for that.

[antonmedv]

How many people are going to actually do import 'zx/global'?

What about package - no globals, zx binary - with globals approach?

Will create a separate PR for after this one.

[antonmedv]

Thoughts about ESM&CJS duality:

• This PR creates (dual package hazard)[https://nodejs.org/api/packages.html#packages_dual_package_hazard] and can cause some difficult to debug bugs.
• See also https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c#gistcomment-3850849

But on other hand it will be definitely easier for projects still in CJS only modes.

[antongolub]

I see it this way:

index.mjs   // contains zx inners
global.mjs  // injects some zx api to global, has own d.ts
cli.mjs // bin entry point: parses args, builds exec context, invokes target script

[antongolub]

Well, dual packages need some additional costs in building and testing. But this is the only way to provide a smooth transition from cjs to esm epoch.

[cspotcode]

#198 avoids the dual package hazard. On the other hand, what dual package hazards do you expect to exist for zx?

You could avoid it by taking the same approach here, writing an .mjs that exposes the .cjs bundle as named exports.

[antonmedv]

global.mjs will not work with bundle everything cjs approach.

[antonmedv]

@cspotcode how in your pr globby is required? How cjs will be able to export it?

[cspotcode]

#198 does a bit of a hack for globby, which does have limitations.

If you await glob(); it will work, because it waits for globby to be imported() async.

If you glob.sync(); without first doing await glob; it'll fail. So that's the limitation / hack. await glob; waits for globby to import()

EDIT: the goal here was to preserve the experience for MJS consumers. They will still have a fully unchanged, native experience. CJS consumers will, at most, need to add await glob; to their code. I was trying to keep everything ESM-first, following your stated philosophy.

---

As I understand it, anything that needs to be cjs compatible simply needs to load synchronously, so it needs to live in a cjs file. Then you can use a mjs shim to expose it as named exports for mjs consumers. The same philosophy should work for global.mjs, right? global.cjs can apply the side-effects to the global object.

[antonmedv]

This is really odd limitation. I prefer dual package 📦

[cspotcode]

You can avoid that limitation and avoid dual package, right?

[antonmedv]

Nope. And we want to develop package as esm for future.

And drop cjs support in one/two tears.

[cspotcode]

I'm saying if the .mjs provides named exports that come from the .cjs, then you avoid the dual-package hazards. mjs consumers get the same API they always have, and cjs support is added. And the code is still written as an ECMAScript module, meaning a clean drop of cjs in the future.

Doesn't matter to me which way it's accomplished, but it's good to understand the issue.

[antonmedv]

I didn’t get it. How?

1. we want to develop esm version
2. Provide cjs
3. Globby is esm only

[cspotcode]

The idea I'm thinking about is:

1. keep index.mjs the way it is, versioned in git. Develop esm version.
2. keep build step compiling index.mjs to index.cjs
3. Do not publish index.mjs. Instead publish imports.mjs which has import lib from './index.cjs'; const {glob, $, ...} = lib; export {glob, $, ...};
4. configure package.json exports so that ESM consumers receive imports.mjs and CJS consumers receive index.cjs

Usage is unchanged for ESM consumers:

import {glob} from 'zx';
await glob();

Usage for TS consumers:

import {glob} from 'zx';
import {foo} from './patterns';
glob.sync(foo);

Usage for CommonJS consumers:

const {glob} = require('zx');
glob.sync();

And require('zx').glob === await import('zx').glob because it's the same code underneath.

It can reduce install size, too, since it ships only a single copy of the code and deps. Though I don't think install size is a concern for a tool like zx.

EDIT fixed code in bullet point 3 at the top

[antonmedv]

I got it. Cool idea. Let's me think it through. Will update PR.

[antonmedv]

Got another problem with my PR. It reexports too much:

[ ⠴
  '$',
  'DEFAULT_FILE_SYSTEM_ADAPTER',
  'FILE_SYSTEM_ADAPTER',
  'ProcessOutput',
  'ProcessPromise',
  '__esModule',
  'argv',
  'cd',
  'chalk',
  'createFileSystemAdapter',
  'default',
  'encloseBrace',
  'escapeLast',
  'escapeNode',
  'escapeRegex',
  'exceedsLimit',
  'exists',
  'fetch',
  'find',
  'flatten',
  'fs',
  'glob',
  'globby',
  'hasRegexChars',
  'isInteger',
  'isInvalidBrace',
  'isObject',
  'isOpenOrClose',
  'isRegexChar',
  'isWindows',
  'nothrow',
  'question',
  'read',
  'reduce',
  'removeBackslashes',
  'removePrefix',
  'sleep',
  'supportsLookbehinds',
  'toPosixSlashes',
  'wrapOutput',
  'write',
  'writev'
]

[antonmedv]

Not really good. :( Supporting both already seems complicated.

[antonmedv]

I was not able to find a solution. So no cjs support, I guess.

[cspotcode]

Why does it export too much? Is that a rollup issue? If so, I can take a look.

…

On Wed, Aug 25, 2021, 1:33 PM Anton Medvedev ***@***.***> wrote: I was not able to find a solution. So no cjs support, I guess. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#203 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC35OGQKSNB5YXIFO3GMLLT6USOHANCNFSM5CVFHSZQ> .

[antonmedv]

Yes, looks like it is rollup issue.

[cspotcode]

Seems like esbuild handles this, and we can use a proper resolve plugin instead of find-and-replace for node: builtins. I reopened and pushed the code to #198.

[antonmedv]

After lots of thinking 💭 🧐 I think this is the best compromise. Like one package inside another. So I'm ok with accepting dual package hazards. rollup produces a much nicer output and requires only Node.js to run (in this case is good, no need for speed here). And mjs is the future prove.

[cspotcode]

Did you figure out a fix for #203 (comment)?

[antonmedv]

Yes, same as you did. Just create and index.cjs file with "filtering".