Adding a CommonJS export

[shahkashani]

👋

First of all, thank you so much for a wonderful library, it's made life so much easier for us.

I ran into some issues while unit testing a component that uses AudioMotionAnalyzer, and this diff is the least intrusive fix I was able to find. It's not 100% ideal, because it expects downstream consumers transpile the library code, but on the upside it does offer a solution and it doesn't make too many changes to the way this library is managed.

What is the issue?

I made a repository to illustrate the problem and there are reproduction steps there: https://github.com/shahkashani/audiomotion-tester

Essentially, our TS/TSX files are often transpiled into CommonJS, but since AudioMotionAnalyzer doesn't offer a CJS export, jest (and eventually babel, when it comes to time to preparing the code for the browser) is unable to find it.

This PR doesn't offer a compiled CommonJS bundle, but it does at least allow a path into creating one. Without this line in package.json, there is no way of making this library work for us as far as I can tell.

If there are alternate solutions or any concerns, please let me know.

Again, thank you so much and keep up the great work!

[hvianna]

Hello! Thanks for adding more info to your issue and for proposing an initial solution.

I would prefer exporting to a UMD-compatible module, for broader compatibility, so I've been looking into using Babel and it's transform-modules-umd but the output is not perfect. You need to add .default to the require statement (and to the global variable for browser usage), which looks really bad. I then found about the add-module-exports plugin, but it doesn't seem to work with the latest versions of Babel preset-env.

I have no experience in generating these old-style modules, so I'm kinda lost here. Would you have any suggestion on how to properly export to UMD format?

[shahkashani]

@hvianna That is totally fair! I took a stab at it. It uses parcel to create the cjs asset, the idea is to do this automatically every time before the library gets pushed to npm (though I haven't tested this particular part of it). I didn't add the dist folder to .gitignore, but maybe it should be there?

Anyway, overall it seems to do the trick for this particular jest problem! Testing steps below.

In this repo:

1. Grab the latest changes committed into this branch
2. Run npm i to pull down Parcel
3. To make sure the script works: npm run build
4. npm link

In https://github.com/shahkashani/audiomotion-tester:

1. git pull origin main
2. npm link audiomotion-analyzer
3. npm run test

I ran the npm start script for audioMotion-analyzer and made sure all the examples still work as-is, but I absolutely may have missed something. For instance, I didn't test if the commonjs version works in a browser (the dist folder isn't even served by the http server right now).

So, if anything seems off, please let me know!

[shahkashani]

Anything I can do to help out with this one? ☺️

[hvianna]

@shahkashani I'll look into it this weekend!

[hvianna]

So the module exported by Parcel also needs an extra .default to work, like so:

const AudioMotionAnalyzer = require('audiomotion-analyzer').default;

Not optimal, but it seems we won't get anything better with the current state of modules and tools.

[shahkashani]

Ah, right! I wonder if exporting a named library (and the existing default for es6) would allow a nicer state of things for cjs? Something like:

export const AudioMotionAnalyzer;
export default AudioMotionAnalyzer;

// cjs
const { AudioMotionAnalyzer } = require('audiomotion-analyzer');
// es6
import AudioMotionAnalyzer from 'audiomotion-analyzer';

[hvianna]

I was looking at the code again and realized that the generated audioMotion-analyzer-cjs.js is still an ES module and not CJS format. It's essentially the original source code with only some local names changed by Parcel, and still using the ES6 native export statement, instead of CJS exports object.

So if that code solved the issue for you, maybe all we need is the "require": "./src/audioMotion-analyzer.js" line in package.json after all? 🤔

I also added a named export to the original module, so const { AudioMotionAnalyzer } = require('audiomotion-analyzer'); now works, at least with Webpack.

Can you please install the new version with npm i audiomotion-analyzer@beta and see if it works for you now? The updated code is also available in the develop branch if you prefer.

[shahkashani]

Thanks so much for taking another look at this!

Out of the box (using this setup), it doesn't seem to work:

But in my Jest config, if I tell Jest to pass things Babel, like so:

index 0e846ae..a116230 100644
--- a/jest.config.js
+++ b/jest.config.js
@@ -3,4 +3,5 @@ module.exports = {
   preset: 'ts-jest',
   testEnvironment: 'jsdom',
   transform: { '^.+\\.[tj]s$': 'ts-jest' },
+  transformIgnorePatterns: ['node_modules/(?!audiomotion-analyzer)'],
 };

...then it does work. So I think that's least an improvement, because it provides at least a path there!

I do wonder though if what Parcel is doing is enough to make things work successfully for cjs, since I was able to get it to run here without passing things through Babel.

[hvianna]

I see you're using import AudioMotionAnalyzer from 'audiomotion-analyzer'; in your test setup.

What if you change that to const { AudioMotionAnalyzer } = require('audiomotion-analyzer'); ?

[shahkashani]

Same problem :(

[hvianna]

Have you tried enabling ESM support on Jest? https://jestjs.io/docs/ecmascript-modules

[shahkashani]

Unfortunately, that seems to make everything act very strangely.

Maybe it's okay though to start with the changes you have in the beta, at least it provides a path for Babel compilation for those who might need it ☺️

[hvianna]

I just pushed 4.5.0-beta.1 to npm, can you give it a try? This is my attempt at generating an UMD module with Babel. Source code is in the umd branch.

Edit: please try it with require without the experimental flag on node. Let's see how it goes.

[shahkashani]

You are amazing. This works like an absolute charm! 🎉

I really appreciate your help on this, and I hope this change doesn't make your workflow more painful!

[hvianna]

Awesome, glad it worked!

@Staijn1 @cprussin @alex-greff If you guys have the chance would you be so kind as to try out the new beta and confirm that it still works in your current environment? I'd like to make sure I didn't break anything for anyone else 😅

[Staijn1]

I've updated my Angular project to use the beta and I see no immediate errors. The deployment of my project is currently running and I will report back if I see any anomalies whilst using Audiomotion, but as of right now everything looks good!

[hvianna]

I've updated my Angular project to use the beta and I see no immediate errors. The deployment of my project is currently running and I will report back if I see any anomalies whilst using Audiomotion, but as of right now everything looks good!

Much appreciated! 🙏

[Staijn1]

I tested it today and did not run into any problems at runtime either. Looks good to me!

[hvianna]

I tested it today and did not run into any problems at runtime either. Looks good to me!

Thanks again!

Stable v4.5.0 should be out next weekend, maybe sooner if I get the chance.

[shahkashani]

Thank you so much again!!