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.

Sign up for GitHub

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

Adds Typings #44

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Adds Typings #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 26 additions & 0 deletions eleventy-fetch.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/// <reference types="node" />
import { Buffer } from 'buffer';

export type FetchType =
| "json"
| "buffer"
| "text";

export type EleventyFetchOptions<TType extends FetchType = "buffer"> = {
type?: TType;
directory?: string;
concurrency?: number;
fetchOptions?: RequestInit;
dryRun?: boolean;
removeUrlQueryParams?: boolean;
verbose?: boolean;
hashLength?: number;
duration?: string;
formatUrlForDisplay?: (url: string) => string;
}

export function EleventyFetch(url: string, options?: EleventyFetchOptions<"buffer">): Buffer;
export function EleventyFetch<T>(url: string, options: EleventyFetchOptions<"json">): T;
export function EleventyFetch(url: string, options: EleventyFetchOptions<"text">): string;
Copy link

Choose a reason for hiding this comment

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

It appears that the return types for these functions should be wrapped in Promise.
This is consistent with how PQueue's add method is typed, which returns a Promise as seen here: https://github.com/sindresorhus/p-queue/blob/main/source/index.ts#L234


export default EleventyFetch;
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"url": "git+https://github.com/11ty/eleventy-fetch.git"
},
"main": "eleventy-fetch.js",
"types": "eleventy-fetch.d.ts",
"scripts": {
"test": "ava",
"sample": "node sample",
Expand Down Expand Up @@ -41,6 +42,7 @@
},
"homepage": "https://github.com/11ty/eleventy-fetch#readme",
"devDependencies": {
"@types/node": "^20.14.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a true dependency, not just for development. Otherwise when you add eleventy-fetch to your normal dependencies, your package manager is unlikely to download @types/node and the type declaration file will become incomplete.

Copy link
Author

Choose a reason for hiding this comment

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

I don't like the idea of declaring this as a full dependency, as that would require all projects, regardless of type support, to pull a rather large type definition. Some bundlers will bundle all dependencies.

While, yes, native TS projects may include this as a dependency, and TS recommends shipping the types in the final bundle, this isn't a native TS project, and most consumers won't be typescript based.

Copy link
Author

Choose a reason for hiding this comment

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

This sums up my experiences: microsoft/types-publisher#81 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s an interesting sumup of the issue! But it does not seem to land at any specific conclusion 🤔

I am confused how the eleventy-fetch.d.ts is supposed to work if it depends on a type (from @types/node) that is not in my local environment. Surely Typescript is just going to tell me that the following code returns with a type any then?

eleventyFetch("", { type: "buffer" });

I do understand the issue of incompatible node types, conflicts with the parent, etc. I am now thinking this should maybe be a peerDependency, potentially set as "@types/node": "*". That way at least the package manager can inform the end-user that there is an unmet requirement when no node types have been loaded.

I think * is a perfectly valid version here, as the code is happy to accept any version of Node.js that supports Buffer.

Copy link
Author

Choose a reason for hiding this comment

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

I think having an error is better than telling typeless users that they are missing a peer dependency or pulling in a large dependency. Again, focusing on the primary user being a user of 11ty - which lacks types (types were accidently removed recently).

The whole, "don't let perfection prevent progress" idea. There aren't any perfect solutions here, so I'm optimizing for not "harming" existing users, while making TS users slightly better.

Another thing to consider, if a TS user needs the buffer api, wouldn't they already have @types/node installed in the consuming project?

"ava": "^5.2.0",
"prettier": "^3.2.5"
},
Expand Down