Web APIs: `File` and `FormData` #1056

kyranet · 2018-10-21T22:19:14Z

This PR adds better Blob spec compliance (objects, Uint16Array, Uint32Array tests), to pass some of WPT's tests.
This also adds File, which extends Blob, check spec here.
And FormData, which I first wanted to do (but later noticed it depended on File).

TODO:

Expand the File and FormData's tests.
~~Implement FetchResponse#formData:~~

deno/js/fetch.ts

Line 159 in c4bddc4

async formData(): Promise<domTypes.FormData> {

I believe this goes out of this PR's scope, I'll do this in another PR once I understand how this method exactly works.

kitsonk

Some thoughts...

kitsonk · 2018-10-22T00:16:09Z

js/form_data.ts

+import { DenoFile } from "./file";
+
+export class FormData implements domTypes.FormData {
+  private data: Array<[string, domTypes.FormDataEntryValue]> = [];


My personal opinion is that we should follow the convention of prefacing private members and methods with _. In TypeScript, because these are not really private, they impact the type structure. Which means that something else that has a public data member is now incompatible structurally, which is undesired. Also, as the TC39 proposal for public and private members becomes a reality, if this is made really private in the future, then there will be a need to refactor and replace the _ sigil with the private member sigil (#) and so having an existing sigil makes that refactoring easier.

Also why is this not a Map?

My personal opinion is that we should follow the convention of prefacing private members and methods with _. In TypeScript, because these are not really private, they impact the type structure. Which means that something else that has a public data member is now incompatible structurally, which is undesired. Also, as the TC39 proposal for public and private members becomes a reality, if this is made really private in the future, then there will be a need to refactor and replace the _ sigil with the private member sigil (#) and so having an existing sigil makes that refactoring easier.

If anything we would use a symbol instead:

deno/js/blob.ts

Lines 5 to 8 in c4bddc4

const bytesSymbol = Symbol("bytes");

export class DenoBlob implements domTypes.Blob {

private readonly [bytesSymbol]: Uint8Array;

Also why is this not a Map?

Because you can "append" multiple elements under the same name, getAll gets a list of all of them. Also because they're in insertion order:

I know using a Map would be awesome, it's superfast and basically what we'd define as idiomatic JavaScript/TypeScript (since append and getAll give a hint of it), but FormData#forEach would log a 1, a 3, b 2 instead of a 1, b 2, a 3 like browsers do. It's kinda sad we can't use a Map here, it would definitely boost FormData's internals by a lot, but we can't afford going out of the standard.

The symbol makes sense. Regarding the Map of course it could be a Map<string, domTypes.FormDataEntryValue[]> and on a .get() you would simply return value[0] and Array.from(value) for getAll() (using Array.from() to ensure to not leak the reference to real value array)

If we were to go performance-wise, I believe Array#slice() (with no arguments) is much faster.

getAll(name: string): domTypes.FormDataEntryValue[] { const entries = this[dataSymbol].get(name); return entries ? entries.slice() : []; }

But then again using the map would be against the implementation of various browsers where FormData#forEach, URLSearchParams#forEach, and others, call the callback strictly by insertion order, which a Map does not allow.

kitsonk · 2018-10-22T09:04:37Z

js/form_data.ts

+   *         console.log(key);
+   *       }
+   */
+  keys(): IterableIterator<string> {


We should produce iterators in a more idiomatic way... I have raised a PR with the way I feel we should be doing them, see #1062.

Feel free to do so, WHATWG defines a series of implementable interfaces like iterable which is essentially like this:

interface IWHATWGIterable<K, V> { keys(): IterableIterator<K>; values(): IterableIterator<V>; entries(): IterableIterator<[K, V]>; [Symbol.iterator](): IterableIterator<[K, V]>; forEach((value: V, key: K, parent: this) => void, thisArg?: any): void; }

We could implement this and use a mixin to implement them, thought that'd need to expose the internal slot, which we cannot do. But if you have a better solution, I'm open to suggestions.

@kyranet I like the idea... I will change my PR to utilise TypeScript mixin classes. The only problem is that it would need to have whatever the internal data is as an iterator, which means you would have to further abstract your array data structure.

@kyranet I pushed an update to #1062 that combines everything we are talking about. Take a look.

kitsonk · 2018-10-22T09:18:58Z

js/file.ts

@@ -0,0 +1,24 @@
+// Copyright 2018 the Deno authors. All rights reserved. MIT license.
+import * as domTypes from "./dom_types";


Hmmm... file is a bit confusing with files. I wonder if this should be structured so that Web API stuff is in a sub folder where things are a bit less confusing, and Blob and Headers and fetch, etc. can go in there.

Indeed, we could have js/webapi/, js/fileapi, js/denoapi, and others. But this is out of the scope of this PR. We can do this later once Event and EventTarget are merged too to prevent merge conflicts/issues with other collaborators working on this.

kitsonk

Some feedback...

kitsonk

Some feedback...

kitsonk

Some feedback...

kitsonk

Couple thoughts...

kitsonk · 2018-10-24T17:58:28Z

js/mixins/dom_iterable.ts

        yield entry;
      }
    }

    *keys(): IterableIterator<K> {
-      for (const key of (this as any)[dataSymbol].keys()) {
-        yield key;
+      for (const entry of (this as any)[dataSymbol]) {


Would personally prefer:

Suggested change

for (const entry of (this as any)[dataSymbol]) {

for (const [key] of (this as any)[dataSymbol]) {

kitsonk · 2018-10-24T17:59:30Z

js/mixins/dom_iterable.ts

      }
    }

    *values(): IterableIterator<V> {
-      for (const value of (this as any)[dataSymbol].values()) {
-        yield value;
+      for (const entry of (this as any)[dataSymbol]) {


Would personally prefer:

Suggested change

for (const entry of (this as any)[dataSymbol]) {

for (const [_key, value] of (this as any)[dataSymbol]) {

kitsonk

LGTM!!!

kitsonk · 2018-10-25T01:13:58Z

js/mixins/dom_iterable.ts

      }
    }

    *values(): IterableIterator<V> {
-      for (const entry of (this as any)[dataSymbol]) {
-        yield entry[1];
+      for (const [, value] of (this as any)[dataSymbol]) {


oooh, yeah forgot you can omit values on array destructuring... 👍

ry

LGTM - thanks!

kyranet added 3 commits October 22, 2018 00:09

feat: File, FormData, and better Blob compliance

fe0044f

spec: Make FormData's iterators spec and web compliant

a6cfbe0

CI: Fixed CI errors and tests

332d78c

kitsonk reviewed Oct 22, 2018

View reviewed changes

style: Make FormData's internal slot a symbol key

48451cf

kitsonk mentioned this pull request Oct 22, 2018

Change DenoHeaders to be more idiomatic TypeScript #1062

Merged

kyranet added 4 commits October 24, 2018 08:34

Merge branch 'master' into file-and-formdata

11c2d1d

src: Use DomIterableMixing and fix for internal array slots

4df5f14

docs: Add comment about the lack of keys() and values()

761ce5e

docs: Use "Additionally" instead of "Alternatively"

be65441

kitsonk reviewed Oct 24, 2018

View reviewed changes

style: Apply @kitsonk's suggestions

84c9ba1

kitsonk reviewed Oct 25, 2018

View reviewed changes

kitsonk mentioned this pull request Oct 25, 2018

Make URLSearchParams compliant with the spec and browser #1055

Closed

ry mentioned this pull request Oct 26, 2018

Fix bundle hashing. #1098

Closed

kyranet added 2 commits October 27, 2018 19:03

src: Added more tests, better spec compliant

5c8f172

Merge branch 'master' into file-and-formdata

5a3e76d

kyranet changed the title ~~[WIP] Web APIs: File and FormData~~ Web APIs: File and FormData Oct 27, 2018

Merge branch 'master' into file-and-formdata

b168faa

ry approved these changes Nov 4, 2018

View reviewed changes

ry merged commit e93d686 into denoland:master Nov 4, 2018

kyranet deleted the file-and-formdata branch November 4, 2018 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web APIs: `File` and `FormData` #1056

Web APIs: `File` and `FormData` #1056

kyranet commented Oct 21, 2018 •

edited

Loading

kitsonk left a comment

kitsonk Oct 22, 2018

kitsonk Oct 22, 2018

kyranet Oct 22, 2018 •

edited

Loading

kitsonk Oct 22, 2018

kyranet Oct 22, 2018

kitsonk Oct 22, 2018

kyranet Oct 22, 2018

kitsonk Oct 22, 2018

kitsonk Oct 22, 2018

kitsonk Oct 22, 2018

kyranet Oct 22, 2018

kitsonk left a comment

kitsonk left a comment

kitsonk left a comment

kitsonk left a comment

kitsonk Oct 24, 2018

kitsonk Oct 24, 2018

kitsonk left a comment

kitsonk Oct 25, 2018

ry left a comment

	const bytesSymbol = Symbol("bytes");

	export class DenoBlob implements domTypes.Blob {
	private readonly [bytesSymbol]: Uint8Array;

		@@ -0,0 +1,24 @@
		// Copyright 2018 the Deno authors. All rights reserved. MIT license.
		import * as domTypes from "./dom_types";

	for (const entry of (this as any)[dataSymbol]) {
	for (const [key] of (this as any)[dataSymbol]) {

	for (const entry of (this as any)[dataSymbol]) {
	for (const [_key, value] of (this as any)[dataSymbol]) {

Web APIs: File and FormData #1056

Web APIs: File and FormData #1056

Conversation

kyranet commented Oct 21, 2018 • edited Loading

TODO:

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyranet Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

Web APIs: `File` and `FormData` #1056

Web APIs: `File` and `FormData` #1056

kyranet commented Oct 21, 2018 •

edited

Loading

kyranet Oct 22, 2018 •

edited

Loading