Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert #409? #419

Closed
saschanaz opened this issue Apr 7, 2018 · 14 comments
Closed

Revert #409? #419

saschanaz opened this issue Apr 7, 2018 · 14 comments

Comments

@saschanaz
Copy link
Contributor

saschanaz commented Apr 7, 2018

  AudioParam setValueCurveAtTime (sequence<float> values, double startTime, double duration);
dictionary WaveShaperOptions : AudioNodeOptions {
  sequence<float> curve;
  OverSampleType oversample = "none";
};

@iccir suggested in #408 that we should use Float32Array citing an older spec, but the latest one says it's sequence<float>.

#409 also changed VRLayer and it also has sequence<float>. (It's now VRLayerInit.)

So I suggest that we revert #409, what do you think? @mhegazy

@iccir
Copy link

iccir commented Apr 7, 2018

I likely pulled up the W3C specification or an old webaudio.github.com one by mistake. This changed in WebAudio/web-audio-api#1189

My original issue was a warning when passing a Float32Array as an argument of setValueCurveAtTime(). From my understanding, this is still allowed by the latest spec ("This is
a backward-compatible change since Float32Array is a sequence…").

Should sequence<float> map to a union type, like number[] | Float32Array?

@saschanaz
Copy link
Contributor Author

saschanaz commented Apr 7, 2018

Should sequence map to a union type, like number[] | Float32Array?

Sequences cannot be used for attributes, so it may make sense.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 7, 2018

@saschanaz what about the other APIs that changed?

Maybe the right fix is to revert the change and add a specific override for setValueCurveAtTime.

Sorry for making the changes without checking the updated spec

@saschanaz
Copy link
Contributor Author

saschanaz commented Apr 8, 2018

what about the other APIs that changed?

I think that's all.

Full diff:

--- a/baselines/dom.generated.d.ts
+++ b/baselines/dom.generated.d.ts
@@ -1033,8 +1033,8 @@ interface PeriodicWaveConstraints {
 }

 interface PeriodicWaveOptions extends PeriodicWaveConstraints {
-    imag?: Float32Array;
-    real?: Float32Array;
+    imag?: number[];
+    real?: number[];
 }

 interface PointerEventInit extends MouseEventInit {
@@ -1569,8 +1569,8 @@ interface VRDisplayEventInit extends EventInit {
 }

 interface VRLayer {
-    leftBounds?: Float32Array | null;
-    rightBounds?: Float32Array | null;
+    leftBounds?: number[] | null;
+    rightBounds?: number[] | null;
     source?: HTMLCanvasElement | null;
 }

@@ -1581,7 +1581,7 @@ interface VRStageParameters {
 }

 interface WaveShaperOptions extends AudioNodeOptions {
-    curve?: Float32Array;
+    curve?: number[];
     oversample?: OverSampleType;
 }

@@ -1984,7 +1984,7 @@ interface AudioParam {
     linearRampToValueAtTime(value: number, endTime: number): AudioParam;
     setTargetAtTime(target: number, startTime: number, timeConstant: number): AudioParam;
     setValueAtTime(value: number, startTime: number): AudioParam;
-    setValueCurveAtTime(values: Float32Array, startTime: number, duration: number): AudioParam;
+    setValueCurveAtTime(values: number[], startTime: number, duration: number): AudioParam;
 }

 declare var AudioParam: {

PeriodicWaveOptions is also from Web Audio.

dictionary PeriodicWaveOptions : PeriodicWaveConstraints {
  sequence<float> real;
  sequence<float> imag;
};

@iccir
Copy link

iccir commented Apr 8, 2018

That reverts it, but the original issue remains: setValueCurveAtTime needs to be able to take a Float32Array without warning.

I'm also not sure what the official policy is regarding specifications that haven't been adopted by all browsers. Firefox and WebKit both require Float32Array and throw an exception on other types.

@saschanaz
Copy link
Contributor Author

saschanaz commented Apr 10, 2018

Another general solution is creating new WebIDL-compatible Sequence<T> type.

The spec says iterable objects can be casted as IDL sequence type, so we may do:

type Sequence<T> = T[] | { [Symbol.iterator](): IterableIterator<T> };

// all of the followings should be allowed

declare function receiveNumberSequence(sequence: Sequence<number>): void;
receiveNumberSequence([0]);
receiveNumberSequence(new Set([0]));
receiveNumberSequence(new Uint8Array([0]));
receiveNumberSequence(new Float32Array([0]));

declare function receiveStringMap(map: Sequence<[string, string]>): void;
receiveStringMap(new Map([["abc", "bcd"]]));
receiveStringMap(new URLSearchParams());
receiveStringMap(new Headers());

I have no idea how to make this ES5-compatible though. microsoft/TypeScript#13031

@mhegazy
Copy link
Contributor

mhegazy commented Apr 10, 2018

ES5 vs ES6 is the issue here..

@mhegazy
Copy link
Contributor

mhegazy commented Apr 10, 2018

So every use of Sequence<float> should be number[] | Float32Array ?

Looks like all the changes in #409 were in input positions, so the use of union should be fine.

@saschanaz
Copy link
Contributor Author

saschanaz commented Apr 10, 2018

So every use of Sequence should be number[] | Float32Array ?

For now, yes.

Looks like all the changes in #409 were in input positions

Actually it's guaranteed by the WIDL spec 😁 Ah, methods can return sequences.

@iccir
Copy link

iccir commented Apr 11, 2018

Is number[] | Float32Array too narrow? Isn't sequence<float> any object with a length that returns float-like values via its subscript operator?

Chrome appears to accept Float64Array and Int16Array values for setValueCurveAtTime(), but I'm not sure if this is what the spec intended.

@saschanaz
Copy link
Contributor Author

That's the intended behavior, but there is no good general way in TS to allow it. Explicitly allowing every possible interfaces will be too exhaustive.

@iccir
Copy link

iccir commented Apr 11, 2018

Makes sense, thanks! number[] | Float32Array (and maybe Float64Array?) should cover most use cases.

@saschanaz
Copy link
Contributor Author

ArrayBufferView may be the best option for those typed arrays.

@iccir
Copy link

iccir commented Apr 11, 2018

ArrayBufferView includes DataView, which I believe isn't allowed in sequence<float> due to the lack of [] and length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants