Change FileReader.readAsText‘s 2nd argument name

[blixt]

Change from label to the more descriptive encoding (MDN docs).

Fixes microsoft/TypeScript#25797

[mhegazy]

I would add the widl file instead. the widl file comes from https://w3c.github.io/FileAPI/#FileReader-interface, to add it see #523 (comment) for more details.

[blixt]

@mhegazy The readAsText entry is already in the WIDL file, I'm merely overriding the non-specific parameter name label to be encoding as it is in e.g., MDN’s documentation.

This could come down to subjective opinion because the W3C chose the name label for the parameter (emphasis mine):

The readAsText() method can be called with an optional parameter, label, which is a DOMString argument that represents the label of an encoding […]

I strongly side with MDN’s documentation choice for the parameter name. My reasoning is that if label is used here, then value, parameter, number, etc would be equally valid argument names for just about any function since these words are only descriptive by proxy.

[blixt]

I also forgot to mention, that the parameter was originally named encoding in this repo, but at some point changed to label (however, seemingly unintended): Commit 30c13a0

[mhegazy]

The name is coming from the spec: https://www.w3.org/TR/FileAPI/#FileReaderSync

It is also in https://w3c.github.io/FileAPI/#APIASynch.

So looks like a spec bug.

@saschanaz which one of the two specs should we be using: https://w3c.github.io/FileAPI/#APIASynch seems to be more up-to-date.

[saschanaz]

which one of the two specs should we be using

The first one is a snapshot and the second one is a nightly version. The latter may frequently go back and forth so I've chosen the former.

[saschanaz]

FYI, you can file an issue for this here: https://github.com/w3c/FileAPI/issues/

[blixt]

Thank you @saschanaz, I've filed such an issue here: w3c/FileAPI#105

[blixt]

The change to the spec is now live: https://w3c.github.io/FileAPI/#idl-index

@mhegazy I'm unable to fetch the updated WIDL due to an error in the npm run fetch command. What is the correct way to update the files to use the new parameter name?

[saschanaz]

npm run fetch should work, but you may do npm install again to make sure you have TS 2.9.x. (See #556).

[blixt]

(I had made another comment here but disregard that since I had the wrong commit checked out, sorry.)

I've run npm run fetch from master now and this is the diff I get, which does not include any changes to FileReader API. What do I need to change?

diff --git a/inputfiles/idl/WebGL 1.widl b/inputfiles/idl/WebGL 1.widl
index 4c5f92b..a6df3de 100644
--- a/inputfiles/idl/WebGL 1.widl	
+++ b/inputfiles/idl/WebGL 1.widl	
@@ -56,36 +56,46 @@ dictionary WebGLContextAttributes {
     GLboolean failIfMajorPerformanceCaveat = false;
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLObject {
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLBuffer : WebGLObject {
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLFramebuffer : WebGLObject {
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLProgram : WebGLObject {
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLRenderbuffer : WebGLObject {
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLShader : WebGLObject {
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLTexture : WebGLObject {
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLUniformLocation {
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLActiveInfo {
     readonly attribute GLint size;
     readonly attribute GLenum type;
     readonly attribute DOMString name;
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLShaderPrecisionFormat {
     readonly attribute GLint rangeMin;
     readonly attribute GLint rangeMax;
@@ -522,7 +532,7 @@ interface mixin WebGLRenderingContextBase
     const GLenum UNPACK_COLORSPACE_CONVERSION_WEBGL = 0x9243;
     const GLenum BROWSER_DEFAULT_WEBGL          = 0x9244;
 
-    readonly attribute HTMLCanvasElement canvas;
+    [Exposed=Window] readonly attribute HTMLCanvasElement canvas;
     readonly attribute GLsizei drawingBufferWidth;
     readonly attribute GLsizei drawingBufferHeight;
 
@@ -730,13 +740,16 @@ interface mixin WebGLRenderingContextBase
     void viewport(GLint x, GLint y, GLsizei width, GLsizei height);
 };
 
+[Exposed=(Window,Worker)]
 interface WebGLRenderingContext
 {
 };
 WebGLRenderingContext includes WebGLRenderingContextBase;
 
 
-[Constructor(DOMString type, optional WebGLContextEventInit eventInit)]
+[Exposed=(Window,Worker),
+ Constructor(DOMString type,
+ optional WebGLContextEventInit eventInit)]
 interface WebGLContextEvent : Event {
     readonly attribute DOMString statusMessage;
 };

[saschanaz]

You can unstage the WebGL changes as it's planned in #561. Here is the expected steps:

1. Update the idlSources.json so that File points to https://w3c.github.io/FileAPI/.
2. npm run fetch and cancel the WebGL changes
3. npm run build && npm run baseline-accept
4. Commit the results

[blixt]

@saschanaz Thank you for the help! Now all has been done and pushed (see new commit).

[saschanaz]

👍

[blixt]

Is there anything else needed to get this merged? :)

[blixt]

@saschanaz Is there anyone else who needs to approve this to get this through?

[saschanaz]

@mhegazy will merge this, but maybe he is busy these days? Hmm.

[blixt]

Sorry to be such a bother but now it's a month since I put up the suggestion to improve one parameter name for the docs and everyone seems to agree it's an improvement. I also went all the way through the W3C to change the parameter name in the spec itself for this. Is there any chance we can get this PR closed? :)

[saschanaz]

Also pinging @RyanCavanaugh.

[blixt]

We're hitting the 3 month mark on this issue and as you can see above other people are starting to go in the same circle I did to fix this issue.

To save everyone time, is there anything we can do to get this merged? The change is compatible with all automated tools and is based on the source of truth (W3C standard) so there shouldn't be anything controversial with the change.

[saschanaz]

It seems there has been some changes in the team organization. Now @DanielRosenwasser may review this.

[sandersn]

I've been asked to take over TSJS-lib-generator, so I hope I can get it back to an up-to-date state soon.

@blixt, one thing to note, I noticed that you changed the source of FileAPI to w3c.github.io from w3.org/TR. This is probably correct for FileAPI since you want to see the new changes. I just want to note it in case it causes too much churn in the future.

[blixt]

Thank you @sandersn! Yes I updated the source based on a suggestion from a repo contributor. I don't know how much churn this will cause but if it's a lot maybe I can look into how and when the W3C puts master into their working draft (https://www.w3.org/TR/FileAPI/).

[saschanaz]

AFAIK the specification is currently pretty stable, so probably won't change too frequently.