bbc / peaks.js

JavaScript UI component for interacting with audio waveforms
https://waveform.prototyping.bbc.co.uk
GNU Lesser General Public License v3.0
3.2k stars 279 forks source link

Type def for peakOptions.webAudio #382

Closed jgentes closed 3 years ago

jgentes commented 3 years ago

Not a big deal, I know you're working on these:

Property 'webAudio' does not exist on type 'OptionalOptions & PreGeneratedWaveformOptions & ViewContainerOptions'

chrisn commented 3 years ago

I'm not sure I see the problem. Can you share the code that causes this error?

jgentes commented 3 years ago

This may be my lack of understanding of Typescript. If I add it directly to the options object, it's fine (no error):

const peakOptions: PeaksOptions = {
    containers: {
      overview: document.getElementById(`overview-container`),
      zoomview: document.getElementById(`zoomview-container`)
    },
    mediaElement: document.getElementById(`audio`),
    pointMarkerColor: 'rgba(30, 139, 195, 1)',
    zoomLevels: [64, 128, 256, 512],
    webAudio = {
      audioContext: new window.AudioContext()
    }
}

However if I try to add it to the object later, it gives the error:

const peakOptions: PeaksOptions = {
    containers: {
      overview: document.getElementById(`overview-container`),
      zoomview: document.getElementById(`zoomview-container`)
    },
    mediaElement: document.getElementById(`audio`),
    pointMarkerColor: 'rgba(30, 139, 195, 1)',
    zoomLevels: [64, 128, 256, 512],
}

peakOptions.webAudio = {
  audioContext: new window.AudioContext()
}

image

chrisn commented 3 years ago

Peaks.js TypeScript definitions has:

type AudioOptions = WebAudioOptions | PreGeneratedWaveformOptions;
export type PeaksOptions = MediaPlayerOptions & ContainerOptions & AudioOptions & OptionalOptions;

When you create your options object, it seems that TypeScript picks the narrowest possible type, so excludes AudioOptions. This means you cannot later add a webAudio attribute.

There may be some useful info here.

I'm not sure how to change the TypeScript definitions to allow what you're trying to do. Perhaps @evanlouie can give some advice?

As a workaround, you can write:

const peaksOptionsWithAudio: PeaksOptions = {
  ...peaksOptions,
  webAudio: {
    audioContext: new window.AudioContext()
  }
};
evanlouie commented 3 years ago

@chrisn hit the needle on the head.

After you first create the PeaksOptions instance, its reifies the discriminated union of AudioOptions to just PreGeneratedWaveformOptions because it has to be flattened for usage in the intersect type of PeaksOptions. This is because the type checker didn't see a webAudio property; so it assumed it was PreGeneratedWaveformOptions.

Sadly, I don't see an easy fix for this just using the type-system.

One potentially low cost change would be:

export type PeaksOptions = OptionalOptions & AudioOptions & ContainerOptions

to something like:

// If you don't care about introducing a breaking change:
export type PeaksOptions = OptionalOptions & { audioOptions: AudioOptions } & ContainerOptions

// If you want a migration path before eventually moving to the above:
export type PeaksOptions = OptionalOptions & (AudioOptions | { audioOptions: AudioOptions }) & ContainerOptions

As AudioOptions would have its own property, the type checker would not need to reify it into a single type and would stay as a discriminated union. This would allow changing of the new audioOptions property to any AudioOptions after creation as the type checker would still know that audioOptions is a AudioOptions.

But this would require a change to the actual code for Peaks.init so that it would parse the AudioOptions from audioOptions.

Overall though, I would recommend @chrisn solution of creating a instance via ... and setting webAudio there. However I am not sure if this will cause any issues in the situation where PreGeneratedWaveformOptions properties were set in the initial instance and you recreated it via spread with a webAudio.

chrisn commented 3 years ago

Thanks @evanlouie, that's really helpful. @jgentes would the spread operator solution work for you?

jgentes commented 3 years ago

Yes, thank you!