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

Web APIs: File and FormData #1056

Merged
merged 12 commits into from
Nov 4, 2018
2 changes: 2 additions & 0 deletions BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,11 @@ ts_sources = [
"js/dom_types.ts",
"js/errors.ts",
"js/fetch.ts",
"js/file.ts",
"js/file_info.ts",
"js/files.ts",
"js/flatbuffers.ts",
"js/form_data.ts",
"js/global_eval.ts",
"js/globals.ts",
"js/io.ts",
Expand Down
8 changes: 8 additions & 0 deletions js/blob.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ function toUint8Arrays(
ret.push(element[bytesSymbol]);
} else if (element instanceof Uint8Array) {
ret.push(element);
} else if (element instanceof Uint16Array) {
const uint8 = new Uint8Array(element.buffer);
ret.push(uint8);
} else if (element instanceof Uint32Array) {
const uint8 = new Uint8Array(element.buffer);
ret.push(uint8);
} else if (ArrayBuffer.isView(element)) {
// Convert view to Uint8Array.
const uint8 = new Uint8Array(element.buffer);
Expand All @@ -105,6 +111,8 @@ function toUint8Arrays(
// Create a new Uint8Array view for the given ArrayBuffer.
const uint8 = new Uint8Array(element);
ret.push(uint8);
} else {
ret.push(enc.encode(String(element)));
}
}
return ret;
Expand Down
4 changes: 2 additions & 2 deletions js/dom_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type ReferrerPolicy =
| "origin-when-cross-origin"
| "unsafe-url";
export type BlobPart = BufferSource | Blob | string;
type FormDataEntryValue = File | string;
export type FormDataEntryValue = File | string;
export type EventListenerOrEventListenerObject =
| EventListener
| EventListenerObject;
Expand Down Expand Up @@ -158,7 +158,7 @@ interface Event {
readonly NONE: number;
}

interface File extends Blob {
export interface File extends Blob {
readonly lastModified: number;
readonly name: string;
}
Expand Down
24 changes: 24 additions & 0 deletions js/file.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2018 the Deno authors. All rights reserved. MIT license.
import * as domTypes from "./dom_types";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import { DenoBlob } from "./blob";

export class DenoFile extends DenoBlob implements domTypes.File {
lastModified: number;
name: string;

constructor(
fileBits: domTypes.BlobPart[],
fileName: string,
options?: domTypes.FilePropertyBag
) {
options = options || {};
super(fileBits, options);

// 4.1.2.1 Replace any "/" character (U+002F SOLIDUS)
// with a ":" (U + 003A COLON)
this.name = String(fileName).replace(/\u002F/g, "\u003A");
// 4.1.3.3 If lastModified is not provided, set lastModified to the current
// date and time represented in number of milliseconds since the Unix Epoch.
this.lastModified = options.lastModified || Date.now();
}
}
106 changes: 106 additions & 0 deletions js/file_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
// Copyright 2018 the Deno authors. All rights reserved. MIT license.
import { test, assert, assertEqual } from "./test_util.ts";
import { DenoFile } from "./file.ts";
import { DenoBlob } from "./blob.ts";

function testFirstArgument(arg1, expectedSize) {
const file = new DenoFile(arg1, "name");
assert(file instanceof DenoFile);
assertEqual(file.name, "name");
assertEqual(file.size, expectedSize);
assertEqual(file.type, "");
}

test(function fileEmptyFileBits() {
testFirstArgument([], 0);
});

test(function fileStringFileBits() {
testFirstArgument(["bits"], 4);
});

test(function fileUnicodeStringFileBits() {
testFirstArgument(["𝓽𝓮𝔁𝓽"], 16);
});

test(function fileStringObjectFileBits() {
// tslint:disable-next-line no-construct
testFirstArgument([new String("string object")], 13);
});

test(function fileEmptyBlobFileBits() {
testFirstArgument([new DenoBlob()], 0);
});

test(function fileBlobFileBits() {
testFirstArgument([new DenoBlob(["bits"])], 4);
});

test(function fileEmptyFileFileBits() {
testFirstArgument([new DenoFile([], "world.txt")], 0);
});

test(function fileFileFileBits() {
testFirstArgument([new DenoFile(["bits"], "world.txt")], 4);
});

test(function fileArrayBufferFileBits() {
testFirstArgument([new ArrayBuffer(8)], 8);
});

test(function fileTypedArrayFileBits() {
testFirstArgument([new Uint8Array([0x50, 0x41, 0x53, 0x53])], 4);
});

test(function fileVariousFileBits() {
testFirstArgument(
[
"bits",
new DenoBlob(["bits"]),
new DenoBlob(),
new Uint8Array([0x50, 0x41]),
new Uint16Array([0x5353]),
new Uint32Array([0x53534150])
],
16
);
});

test(function fileNumberInFileBits() {
testFirstArgument([12], 2);
});

test(function fileArrayInFileBits() {
testFirstArgument([[1, 2, 3]], 5);
});

test(function fileObjectInFileBits() {
// "[object Object]"
testFirstArgument([{}], 15);
});

function testSecondArgument(arg2, expectedFileName) {
const file = new DenoFile(["bits"], arg2);
assert(file instanceof DenoFile);
assertEqual(file.name, expectedFileName);
}

test(function fileUsingFileName() {
testSecondArgument("dummy", "dummy");
});

test(function fileUsingSpecialCharacterInFileName() {
testSecondArgument("dummy/foo", "dummy:foo");
});

test(function fileUsingNullFileName() {
testSecondArgument(null, "null");
});

test(function fileUsingNumberFileName() {
testSecondArgument(1, "1");
});

test(function fileUsingEmptyStringFileName() {
testSecondArgument("", "");
});
173 changes: 173 additions & 0 deletions js/form_data.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
// Copyright 2018 the Deno authors. All rights reserved. MIT license.
import * as domTypes from "./dom_types";
import { CreateIterableIterator } from "./util";
import { DenoBlob } from "./blob";
import { DenoFile } from "./file";

export class FormData implements domTypes.FormData {
private data: Array<[string, domTypes.FormDataEntryValue]> = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also why is this not a Map?

Copy link
Contributor Author

@kyranet kyranet Oct 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/** Appends a new value onto an existing key inside a `FormData`
* object, or adds the key if it does not already exist.
*
* formData.append('name', 'first');
* formData.append('name', 'second');
*/
append(name: string, value: string): void;
append(name: string, value: DenoBlob, filename?: string): void;
append(name: string, value: string | DenoBlob, filename?: string): void {
if (value instanceof DenoBlob) {
const file = new DenoFile([value], filename || name);
this.data.push([name, file]);
} else {
this.data.push([name, value]);
}
}

/** Deletes a key/value pair from a `FormData` object.
*
* formData.delete('name');
*/
delete(name: string): void {
let i = 0;
while (i < this.data.length) {
if (this.data[i][0] === name) {
this.data.splice(i, 1);
} else {
i++;
}
}
}

/** Returns an array of all the values associated with a given key
* from within a `FormData`.
*
* formData.getAll('name');
*/
getAll(name: string): domTypes.FormDataEntryValue[] {
const values = [];
for (const entry of this.data) {
if (entry[0] === name) {
values.push(entry[1]);
}
}

return values;
}

/** Returns the first value associated with a given key from within a
* `FormData` object.
*
* formData.get('name');
*/
get(name: string): domTypes.FormDataEntryValue | null {
for (const entry of this.data) {
if (entry[0] === name) {
return entry[1];
}
}

return null;
}

/** Returns a boolean stating whether a `FormData` object contains a
* certain key/value pair.
*
* formData.has('name');
*/
has(name: string): boolean {
return this.data.some(entry => entry[0] === name);
}

/** Sets a new value for an existing key inside a `FormData` object, or
* adds the key/value if it does not already exist.
*
* formData.set('name', 'value');
*/
set(name: string, value: string): void;
set(name: string, value: DenoBlob, filename?: string): void;
set(name: string, value: string | DenoBlob, filename?: string): void {
this.delete(name);
if (value instanceof DenoBlob) {
const file = new DenoFile([value], filename || name);
this.data.push([name, file]);
} else {
this.data.push([name, value]);
}
}

/** Calls a function for each element contained in this object in
* place and return undefined. Optionally accepts an object to use
* as this when executing callback as second argument.
*
* formData.forEach((value, key, parent) => {
* console.log(value, key, parent);
* });
*/
forEach(
callbackfn: (
value: domTypes.FormDataEntryValue,
key: string,
parent: FormData
) => void,
// tslint:disable-next-line:no-any
thisArg?: any
) {
if (typeof thisArg !== "undefined") {
callbackfn = callbackfn.bind(thisArg);
}
for (const [key, value] of this.entries()) {
callbackfn(value, key, this);
}
}

/** Returns an iterator allowing to go through all keys contained
* in this object.
*
* for (const key of formData.keys()) {
* console.log(key);
* }
*/
keys(): IterableIterator<string> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const list = this.data.map(entry => entry[0]);
const iterators = list.values();
return new CreateIterableIterator(iterators);
}

/** Returns an iterator allowing to go through all values contained
* in this object.
*
* for (const value of formData.values()) {
* console.log(value);
* }
*/
values(): IterableIterator<domTypes.FormDataEntryValue> {
const list = this.data.map(entry => entry[1]);
const iterators = list.values();
return new CreateIterableIterator(iterators);
}

/** Returns an iterator allowing to go through all key/value
* pairs contained in this object.
*
* for (const [key, value] of formData.entries()) {
* console.log(key, value);
* }
*/
entries(): IterableIterator<[string, domTypes.FormDataEntryValue]> {
const iterators = this.data.values();
return new CreateIterableIterator(iterators);
}

/** Returns an iterator allowing to go through all key/value
* pairs contained in this object.
*
* for (const [key, value] of formData[Symbol.iterator]()) {
* console.log(key, value);
* }
*/
[Symbol.iterator](): IterableIterator<[string, domTypes.FormDataEntryValue]> {
const iterators = this.data.values();
return new CreateIterableIterator(iterators);
}
}
Loading