Closed jstewmon closed 3 years ago
@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?
@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.
@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.
@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."
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 !
:tada:
Can we get a release soon? I have time this week-end to open PRs against reverse-dependencies.
@lilred done!
Thank you very much!
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. :(
@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.
This PR demonstrates how we could address #70 with ts-graft.