Allow null on mediaElement

[jgentes]

attn: @evanlouie

The current type definition of mediaElement is Element, however if you assign document.querySelector() or document.getElementById() to mediaElement, they return Element | null. This change adjusts the type definition to account for this and properly allow these selectors.

Note: I don't know if actually having a null value for mediaElement would cause other issues :/

[jgentes]

Also I would be curious to know why HTMLElement isn't allowed here, as document.getElementbyId() returns an HTMLElement. I'm guessing Element will allow HTMLElement?

[evanlouie]

Note: I don't know if actually having a null value for mediaElement would cause other issues :/

I'm not sure either actually. To err on maintaining more concise type definitions, I would not recommend expanding the type definition, I would instead recommend adjusting your setting of the mediaElement property be something like:

{ mediaElement: document.querySelector(<your-selector>) ?? undefined, }

Also I would be curious to know why HTMLElement isn't allowed here, as document.getElementbyId() returns an HTMLElement. I'm guessing Element will allow HTMLElement?

HTMLElement extends Element, so all HTMLElement can be used as Element. From lib.dom.d.ts from typescript core:

interface HTMLElement extends Element, DocumentAndElementEventHandlers, ElementCSSInlineStyle, ElementCSSInlineStyle, ElementContentEditable, GlobalEventHandlers, HTMLOrSVGElement {
...
}

[chrisn]

Passing null for mediaElement is not valid - it must be a valid HTMLMediaElement, otherwise you'll get a runtime error via the Peaks.init() callback.

[chrisn]

To be more precise, you need to pass either an HTMLMediaElement for mediaElement or the player option should contain a Player object. I guess we could make the Typescript declarations convey that in a better way than having mediaElement and player both optional.

[jgentes]

@evanlouie it looks like using mediaElement: document.getElementById('audio1') ?? undefined results in a similar warning:

Types of property 'mediaElement' are incompatible.
  Type 'HTMLElement | undefined' is not assignable to type 'Element'.
    Type 'undefined' is not assignable to type 'Element'.ts(2345)

[evanlouie]

@jgentes not sure why you're getting that... both of these work for me:

import { PeaksOptions } from "peaks.js";

const optionsOne: PeaksOptions = {
  mediaElement: document.querySelector("#foo") ?? undefined,
  containers: {},
};

const optionsTwo: PeaksOptions = {
  mediaElement: document.getElementById("foo") ?? undefined,
  containers: {},
};

[jgentes]

Odd, I'm on version 0.23.1 - same for you? Why would undefined work if not allowed by Element? (i'm relatively new to TS)

Here's my VSCode:

[jgentes]

I guess this could be a solution: mediaElement: document.getElementById('audio1')!

I'm also getting tripped up on Peaks.init - maybe there's an example of how this is done?

Peaks.init(peakOptions, (err: Error, waveform: PeaksInstance): void => {})

Throws a warning:

Argument of type '(err: Error, waveform: PeaksInstance) => void' is not assignable to parameter of type 'PeaksInitCallback'.
  Types of parameters 'waveform' and 'peaks' are incompatible.
    Type 'PeaksInstance | undefined' is not assignable to type 'PeaksInstance'.
      Type 'undefined' is not assignable to type 'PeaksInstance'.ts(2345)

And if I try Peaks.init(peakOptions, (err: Error, waveform: PeaksInstance): PeaksInitCallback => {}) I'm not sure how to return a value that will be of type PeaksInitCallback

[evanlouie]

@jgentes This is the test setup I have:

package.json:

{
  "name": "peaks-test",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "peaks.js": "^0.24.0",
    "typescript": "^4.2.3"
  }
}

tsconfig.json

{
  "compilerOptions": {
    "target": "es5",
    "module": "commonjs",
    "strict": true,
    "esModuleInterop": true,
    "skipLibCheck": true,
    "forceConsistentCasingInFileNames": true
  }
}

test.ts

import Peaks, { PeaksInstance, PeaksOptions } from "peaks.js";

const optionsOne: PeaksOptions = {
  mediaElement: document.querySelector("#foo") ?? undefined,
  containers: {},
};

const optionsTwo: PeaksOptions = {
  mediaElement: document.getElementById("foo") ?? undefined,
  containers: {},
};

// callback based
const initPeaks = () => {
  Peaks.init(
    optionsTwo,
    ((err, peaks) => {
      if (err) {
        // do something to error
        console.error(err);
      }
      // you know have access to the peaks instance
      console.log(peaks);
    }),
  );
};

// or if you like Promises
const asyncInitPeaks = async (): Promise<PeaksInstance> => {
  return new Promise((resolve, reject) => {
    Peaks.init(
      optionsTwo,
      (err, peaks) => {
        if (err) {
          return reject(Error(`initializing peaks: ${err}`));
        }
        if (typeof peaks === "undefined") {
          // note this line should never be reached as it peaks will only be undefined when there is an error.
          // this line is purely to refine the type of the peaks object to {PeaksInstance}
          return reject(
            Error(`undefined peaks object returned to initialization callback`),
          );
        }
        return resolve(peaks);
      },
    );
  });
};

[jgentes]

As it turns out, my file has JSX in it, so it is a .tsx file. If I rename it to .ts the typescript errors I'm reporting go away.

I'll see if I can figure out why there's a difference 🤷

[chrisn]

Thanks @jgentes. This has prompted me to take a closer look at the project's TypeScript definitions file to see if there are any improvements we can make, so thank you!

[jgentes]

Final follow up:

The fix was upgrading to v24.0.. when I downgraded back to v23.1, the error came back, upgraded again and it went away :)

Sorry, should have upgraded before submitting this, especially since I needed some of the other features in this release!