hvianna / audioMotion-analyzer

High-resolution real-time graphic audio spectrum analyzer JavaScript module with no dependencies.
https://audioMotion.dev
GNU Affero General Public License v3.0
619 stars 62 forks source link

Strange type definition for GradientColorStop #37

Closed s-moran closed 1 year ago

s-moran commented 1 year ago

The type definition for GradientColorStop is giving me troubles when trying to define a simple gradient using a string array:

    const gradientOptions = {
      bgColor: '#011a35', 
      colorStops: [      
        '#dadfff',         
        '#f002c7'
      ]
    };
    audioMotion.registerGradient('fsr', gradientOptions);

error TS2345: Argument of type '{ bgColor: string; colorStops: string[]; }' is not assignable to parameter of type 'GradientOptions'. Types of property 'colorStops' are incompatible. Type 'string[]' is not assignable to type 'ArrayTwoOrMore'. Type 'string[]' is missing the following properties from type '{ 0: GradientColorStop; 1: GradientColorStop; }': 0, 1

Edit: Likely due to following type definition in src/index.d.ts:

type ArrayTwoOrMore<T> = {
  0: T
  1: T
} & Array<T>;
Staijn1 commented 1 year ago

Hello,

I have created this type in a pull-request some time ago. I've replicated your scenario and I get the same error, but there's a simple fix! Just add the GradientOptions type to your object. This results in the following:

const gradientOptions: GradientOptions = {
      bgColor: '#011a35', 
      colorStops: [      
        '#dadfff',         
        '#f002c7'
      ]
    };
audioMotion.registerGradient('fsr', gradientOptions);

To explain a bit more on why this is happening: Colorstops is an array that:

Because you did not explicitly enter a type for your gradientOptions variable, Typescript will create a type from the object you defined. The resulting type has the string[] type for colorStops.

Typescript is not smart enough to see a string[] (containing two elements) is the same as ArrayTwoOrMore

@hvianna As this created some confusion, we could revert my pull-request. This means typescript will no longer trip on an array that only has one element, but it will prevent these issues. What do you think?

s-moran commented 1 year ago

@Staijn1 Thanks. I was able to resolve it with your syntax. I thought I did try explicitly casting it before, but I must have made an error :).

However, also keep in mind that I had to import that type using the snippet below, which might not be obvious to all.

import { GradientOptions } from 'audiomotion-analyzer/src/index.d';
Staijn1 commented 1 year ago

Hi there! I'm glad you were able to fix your issue.

Regarding your import, this seems to work for me:

import {GradientOptions} from 'audiomotion-analyzer';
s-moran commented 1 year ago

@Staijn1 Yes, you're correct. This was my final working import (was unable to do in one line)

import AudioMotionAnalyzer from 'audiomotion-analyzer';
import { GradientOptions } from 'audiomotion-analyzer';

We can close this issue now.

Staijn1 commented 1 year ago

Hi there,

If you'd really want to you can do it in one line, like this:

import AudioMotionAnalyzer, { GradientOptions } from 'audiomotion-analyzer'

But what works, works :)

s-moran commented 1 year ago

Ah thanks :)

hvianna commented 1 year ago

Thanks @s-moran for reporting and thank you @Staijn1 for your help on this issue.

I just realized I need to improve the documentation to include the TS types.

I hope I can resume working on this project soon. Thanks again guys!

hvianna commented 1 year ago

@Staijn1

import AudioMotionAnalyzer, { GradientOptions } from 'audiomotion-analyzer'

So I did a quick release last night to fix a boo-boo I made in the TS file a while ago, and I took this opportunity to add the code above to the registerGradient() documentation.

As I'm not really a TS user, I'd appreciate your opinion on whether we should change the GradientOptions interface in a future 4.0.0 release. Also, is it useful to expose (export) other types/interfaces (examples below)? Is there any disadvantage in exporting them all?

interface AnalyzerBarData {
  posX: number;
  freq: number;
  freqLo: number;
  freqHi: number;
  hold: [ number, number? ];
  peak: [ number, number ];
  value: [ number, number? ];
}

interface ConstructorOptions extends Options {
  audioCtx?: AudioContext;
  connectSpeakers?: boolean;
  fsElement?: HTMLElement;
  source?: HTMLMediaElement | AudioNode;
}

type EnergyPreset = "peak" | "bass" | "lowMid" | "mid" | "highMid" | "treble";

type GradientColorStop = string | { pos: number; color: string };

type WeightingFilter = "" | "A" | "B" | "C" | "D" | "468";

Thanks!

Staijn1 commented 1 year ago

Hi,

I believe exporting all interfaces would probably be a good idea, because it would make the imports a bit more intuitive.

The example you've written would become:

import {AudioMotionAnalyzer, GradientOptions } from 'audiomotion-analyzer'
hvianna commented 1 year ago

As of v4.0.0-beta.4 gradients can define a single color, so the interface will be simplified.