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

added fetch api type definitions extracted from official dom lib using ts-graft #93

Merged
merged 1 commit into from Mar 12, 2021

Conversation

jstewmon
Copy link
Contributor

This PR demonstrates how we could address #70 with ts-graft.

.gitignore Outdated
@@ -2,6 +2,7 @@
*.log
test.ponyfill.js
test.polyfill.js
dom.d.ts
Copy link
Owner

Choose a reason for hiding this comment

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

not sure why we would ignore this file. will it change every time ts-graft runs?

package.json Outdated
@@ -77,13 +77,16 @@
"serve-index": "1.9.1",
"sinon": "9.2.4",
"standard": "16.0.3",
"ts-graft": "1.0.1",
"typescript": "4.2.3",
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need the typescript dependency here.

Makefile Outdated
@@ -4,7 +4,7 @@ node_modules: package.json
npm install && /usr/bin/touch node_modules

build: node_modules
npx rollup -c && /usr/bin/touch dist
npx ts-graft && npx rollup -c && /usr/bin/touch dist
Copy link
Owner

Choose a reason for hiding this comment

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

this feels overkill to me. there's no need to run that command on every build.

.ts-graftrc.js Outdated
grafts: [
{
source: "typescript/lib/lib.dom.d.ts",
output: "dom.d.ts",
Copy link
Owner

Choose a reason for hiding this comment

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

let's call it lib.fetch.d.ts

@jstewmon
Copy link
Contributor Author

@lquixada thanks for the speedy review!

The typescript library needs to be available to provide typescript/lib/lib.dom.d.ts. If we don't add typescript to the dev dependencies, when someone wants to regenerate our lib.fetch.d.ts file, they'll have to BYO typescript library.

WRT to change frequency, the output will only change if the version of typescript changes and the new version has a different lib/lib.dom.d.ts file. I wouldn't expect the output to change often, but it is plausible / legitimate.

Admittedly, it is inefficient to generate the file on every build, but that is the easy way of demonstrating its provenance. If someone were inclined to publish the output of lib files as a library, we could just consume the transformed version as a plain type definition file. That would be nice...

If you prefer that I generate and commit lib.fetch.d.ts, which it sounds like you do, would you like me to add a section to the readme to explain how to regenerate it?

@lquixada
Copy link
Owner

@jstewmon giving it a second thought here. Automation is good but it brings overhead: dependencies, configurations, increased build time. I feel we should keep things as simple as possible by just adding the generated lib.fetch.d.ts file with the reviewed index.d.ts on this PR.

Depending on how often the type definition changes, we could introduce ts-graft to automate that process. Hopefully at that point, that tool will be more mature and the output file (lib.fetch.d.ts) will have been battle-tested.

@jstewmon
Copy link
Contributor Author

@lquixada SGTM. I squashed everything down to one commit that I hope reflects what you wanted.

I included a small shell "one-liner" in the commit message that can be used to reproduce lib.fetch.d.ts without leaving behind any additional artifacts. If you want, I could add a new make target of it for the future, so it isn't buried in the commit history.

@lquixada
Copy link
Owner

@jstewmon LGTM! I just need the commit message and the PR title to reflect the new changes. Something like "added fetch api type definitions extracted from official dom lib using ts-graft."

…g ts-graft

lib.fetch.d.ts includes all types from the dom lib needed to define the
fetch function and the Request, Response, and Headers classes. The file
was generated by ts-graft as follows:

npm i --no-save typescript@4.2.3 \
&& cat <<EOF > .ts-graftrc.yaml \
&& npx ts-graft@1.0.1 \
&& rm .ts-graftrc.yaml \
&& npm un typescript
grafts:
- source: typescript/lib/lib.dom.d.ts
  output: lib.fetch.d.ts
  include: [RequestInfo, RequestInit, Response]
EOF
@jstewmon jstewmon changed the title chore: use ts-graft to include requisite types from dom lib added fetch api type definitions extracted from official dom lib using ts-graft Mar 12, 2021
@jstewmon jstewmon requested a review from lquixada March 12, 2021 02:40
@lquixada
Copy link
Owner

Although the security check failed, it is due to a limitation on Github Actions passing secrets to PRs on forks. I'm merging it any way as it will get checked on the main branch.

PR looks great! Thanks @jstewmon !

@carlpaten
Copy link

🎉

Can we get a release soon? I have time this week-end to open PRs against reverse-dependencies.

@lquixada
Copy link
Owner

@LilRed done!

@carlpaten
Copy link

Thank you very much!

@pitgrap
Copy link

pitgrap commented Mar 18, 2021

We're using Typescript and this PR and release is breaking our build.

../../node_modules/cross-fetch/index.d.ts:18:14 - error TS2304: Cannot find name 'BodyInit'.

18   new(body?: BodyInit | null, init?: ResponseInit): Response;
                ~~~~~~~~

../../node_modules/cross-fetch/index.d.ts:18:38 - error TS2304: Cannot find name 'ResponseInit'.

18   new(body?: BodyInit | null, init?: ResponseInit): Response;
                                        ~~~~~~~~~~~~

../../node_modules/cross-fetch/index.d.ts:25:14 - error TS2304: Cannot find name 'HeadersInit'.

25   new(init?: HeadersInit): Headers;

BodyInit and HeadersInit are missing in the import, but ResponseInit is not existing at all. :(

@jstewmon
Copy link
Contributor Author

@pitgrap Sorry about that. #96 should resolve - I added a compilation check to the shell command I used to generate the lib file.

The problem was introduced when I copied the constructor signatures. The missing types weren't reachable from the interface definitions, and I forgot to list the types used by those signatures in the graft config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants