Distribute CommonJS for projects that cannot use ESM

[ReallyBadNews]

Pull request for @cloudinary/url-gen

What does this PR solve?

ES6 is great, and we all love writing it but we have several projects that cannot consume ESM and in which we cannot add additional build steps to transpile ES6 code to a suitable format.

This PR builds CommonJS in lieu of ESM via the Typescript compiler.

Final checklist

• Implementation is aligned to Spec.
• Tests - Add proper tests to the added code.

[patrick-tolosa]

hey @ReallyBadNews

Thanks for the PR.
The package already contains a UMD package:

import {Cloudinary} from '@cloudinary/url-gen/bundles/umd/base.js'
You can import a commonJS alternative from this path.

In fact, if you're using a modern NodeJS version and the require syntax (for commonJS), this should happen automatically based on the package json export fields : https://github.com/cloudinary/js-url-gen/blob/master/package.json#L114

[ReallyBadNews]

Hi @patrick-tolosa, thanks for the response. I did get the UMD bundle working with our application. It does appear however that the typescript definitions are broken for those imports, and we obviously lose any ability to do tree-shaking with only a single entry point.

The typescript defs work when importing from @cloudinary/url-gen/bundles/umd, which is incorrect but break when using the correct @cloudinary/url-gen/bundles/umd/base import.

https://codesandbox.io/s/quiet-sound-453yf?file=/pages/index.tsx

[patrick-tolosa]

Please open an issue regarding the incorrect types for the UMD package, (since this PR doesn't discuss or resolve this issue)

Regarding tree shaking, from what I understand your feature request is to add a CommonJS separate bundle to allow for individual imports.

If you'd like to modify this PR to support that we'll be happy to take a look, in any case this is something we'll take internally to discuss and design the right approach.

[ReallyBadNews]

@patrick-tolosa I went ahead and opened #445 for the umd typescript definitions.

I updated the PR to support both CommonJS and ESM via rollup . It keeps the same dist structure while changing the package.json's "type": "module" so node will assume that .js files are esm, while the .cjs files are interpreted as CommonJS.

The package.json's conditional exports and the corresponding function that generates those have been updated to support the import / require syntax

Thanks again, and please let me know if you folks have any concerns or code review.

[patrick-tolosa]

Right, so this PR does a few things (Not against it, just to keep track).

1. Modify the rollup build to be responsible for both Typescript and UMD.
2. Add CJS output to the rollup build.
3. Expose the CJS files through package.exports property.

All in all this looks good, but can you please consider taking a look at the broken build?
https://app.travis-ci.com/github/cloudinary/js-url-gen/builds/238872032

[ReallyBadNews]

@patrick-tolosa to clarify

1. Rollup handles UMD, CJS, and ESM
2. tsconfig set to emitDeclarationOnly, only generating the d.ts files. TS doesn't support support mjs just yet, but seems like it is coming in v4.5 TS#4455, TS#18442
3. Yup!

I assumed that the failing build was the bundle size tests, I'll look into that!

EDIT: those tests are passing now.

[strausr]

@ReallyBadNews we reviewed the latest changes and they look great but we would like for the bundle size to not increase. We would also prefer not to modify the original build process.
Can you add your changes in addition to what we have, instead of modifying the build itself?

[ReallyBadNews]

@strausr, I went back to using the typescript compiler for the esm output, so all the bundle size tests pass at their original thresholds and the esm build process remains unmodified.

I've also updated the Rollup plugin packages to use the newer @rollup scope, as the ones in use have been deprecated and are no longer maintained.

[patrick-tolosa]

@ReallyBadNews - Thanks for your contribution!

We've finished our review process and we'll release this addition in the next coming release.

[ReallyBadNews]

Thanks @patrick-tolosa and @strausr I appreciated the help!