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

refactor!: split file input change event into seperate function #129

Open
wants to merge 1 commit into
base: beta
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,11 @@ document.addEventListener("drop", async (event) => {
Convert a [change event](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event) for an input type file to File objects:

```js
import { fromEvent } from "file-selector";
import { fromChangeEvent } from "file-selector";

const input = document.getElementById("myInput");
input.addEventListener("change", async (event) => {
const files = await fromEvent(event);
input.addEventListener("change", (event) => {
const files = fromChangeEvent(event);
console.log(files);
});
```
Expand Down
63 changes: 38 additions & 25 deletions src/file-selector.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,42 @@
import { FileWithPath } from "./file.js";
import { fromEvent, fromFileHandles } from "./file-selector.js";
import {
fromChangeEvent,
fromEvent,
fromFileHandles,
} from "./file-selector.js";

describe("fromChangeEvent", () => {
it("returns the files from the event", () => {
const name = "test.json";
const mockFile = createFile(
name,
{ ping: true },
{
type: "application/json",
},
);
const evt = inputEvtFromFiles(mockFile);

const files = fromChangeEvent(evt);
expect(files).toHaveLength(1);
expect(files.every((file) => file instanceof File)).toBe(true);

const [file] = files as FileWithPath[];

expect(file.name).toBe(mockFile.name);
expect(file.type).toBe(mockFile.type);
expect(file.size).toBe(mockFile.size);
expect(file.lastModified).toBe(mockFile.lastModified);
expect(file.path).toBe(`./${name}`);
});

it("throws if the event is not associated with a file input", () => {
const event = new Event("change");
expect(() => fromChangeEvent(event)).toThrow(
"Event is not associated with a valid file input element.",
);
});
});

it("returns a Promise", async () => {
const evt = new Event("test");
Expand All @@ -16,30 +53,6 @@ it("should return an empty array if drag event", async () => {
expect(files).toHaveLength(0);
});

it("should return the evt {target} {files} if the passed event is an input evt", async () => {
const name = "test.json";
const mockFile = createFile(
name,
{ ping: true },
{
type: "application/json",
},
);
const evt = inputEvtFromFiles(mockFile);

const files = await fromEvent(evt);
expect(files).toHaveLength(1);
expect(files.every((file) => file instanceof File)).toBe(true);

const [file] = files as FileWithPath[];

expect(file.name).toBe(mockFile.name);
expect(file.type).toBe(mockFile.type);
expect(file.size).toBe(mockFile.size);
expect(file.lastModified).toBe(mockFile.lastModified);
expect(file.path).toBe(`./${name}`);
});

it("should return an empty array if the evt {target} has no {files} prop", async () => {
const evt = inputEvtFromFiles();
const files = await fromEvent(evt);
Expand Down
45 changes: 29 additions & 16 deletions src/file-selector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,32 @@ export async function fromEvent(
): Promise<(FileWithPath | DataTransferItem)[]> {
if (isObject<DragEvent>(evt) && isDataTransfer(evt.dataTransfer)) {
return getDataTransferFiles(evt.dataTransfer, evt.type);
} else if (isChangeEvt(evt)) {
return getInputFiles(evt);
}
return [];
}

function isDataTransfer(value: any): value is DataTransfer {
return isObject(value);
}

function isChangeEvt(value: any): value is Event {
return isObject<Event>(value) && isObject(value.target);
}

function isObject<T>(v: any): v is T {
return typeof v === "object" && v !== null;
}

function getInputFiles(event: Event): FileWithPath[] {
/**
* Retrieves files from the `change` event of a file input element.
*
* @param event The `change` event of a file input element to retrieve files from.
* @throws If the event is not associated with a valid file input element.
* @returns An array of file objects retrieved from the event.
*
* @example
* ```js
* const input = document.getElementById("myInput");
* input.addEventListener("change", (event) => {
* const files = fromChangeEvent(event);
* console.log(files);
* });
* ```
*
* @see {@link https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file|MDN - `<input type="file">`}
* @see {@link https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event|MDN - HTMLElement: `change` event}
*/
export function fromChangeEvent(event: Event): FileWithPath[] {
if (!(event.target instanceof HTMLInputElement) || !event.target.files) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we also check with isObject? Since we don't know what to expect at runtime.

Suggested change
if (!(event.target instanceof HTMLInputElement) || !event.target.files) {
if (!isObject(event) || !(event.target instanceof HTMLInputElement) || !event.target.files) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that is needed, we already have a very specific type of Event, so this value should always be correct. Of course users can pass whatever they want if they are not using TypeScript, but then they are still violating the API contract, and it is their responsibility to do this check.

return [];
throw new Error("Event is not associated with a valid file input element.");
}

return Array.from(event.target.files).map((file) => toFileWithPath(file));
Expand Down Expand Up @@ -72,6 +77,14 @@ async function fromFileHandle(
return toFileWithPath(file);
}

function isDataTransfer(value: any): value is DataTransfer {
return isObject(value);
}

function isObject<T>(v: any): v is T {
return typeof v === "object" && v !== null;
}

async function getDataTransferFiles(dataTransfer: DataTransfer, type: string) {
const items = Array.from(dataTransfer.items).filter(
(item) => item.kind === "file",
Expand Down
6 changes: 5 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
export { fromEvent, fromFileHandles } from "./file-selector.js";
export {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do believe that providing a convenience function that runs though each would help users migrate smoothly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am going to rename and refactor fromEvent to something else as well, perhaps we can then evaluate adding it back and see if it is worth to keep it in a deprecated manner?

I have to do some digging as well and see how we're going to go about upgrading this dependency in the main project, so there will likely be a beta branch there as well.

fromChangeEvent,
fromEvent,
fromFileHandles,
} from "./file-selector.js";
export { FileWithPath } from "./file.js";