Skip to content

💥 Add strong type definition for fetch().json() #26

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

Merged
merged 4 commits into from
Apr 5, 2023

Conversation

aaditmshah
Copy link
Contributor

Closes #24. You can review this PR on a per-commit basis. Here are the main changes that I made.

  1. I used the TypeScript/src/lib/libs.json file to get the list of lib files.
  2. I split the lib files into source files and target files.
  3. This change was necessary to generate the lib.dom.d.ts file.
  4. I ran the build:lib and build:diff commands to regenerate the assets.
  5. I removed @typescript/lib-header from the list of dependencies in the tests directory.
  6. I added a strong type definition and a corresponding test for fetch().json().

@aaditmshah
Copy link
Contributor Author

@uhyo I don't know why the check is failing. It's working for me locally.

@uhyo
Copy link
Owner

uhyo commented Apr 4, 2023

I even got a different error. Let me try upgrading Node.js version on CI first 😨

@uhyo
Copy link
Owner

uhyo commented Apr 4, 2023

@aaditmshah Running npm install in the tests directory seems to produce a diff in package-lock.json. Could you try that?

Anyway thank you for this! The first improvement in lib-dom would be absolutely nice.

@aaditmshah
Copy link
Contributor Author

@aaditmshah Running npm install in the tests directory seems to produce a diff in package-lock.json. Could you try that?

Downgraded to node version 16 and generated a package-lock.json version 2 file. The continuous integration test is passing now.

We should probably add an .nvmrc file to the repository. What do you think?

@uhyo
Copy link
Owner

uhyo commented Apr 4, 2023

Oh actually using version 3 would be file. I guess there was just something wrong with original commit. I tested locally with latest npm and it seems to work. Also Node.js on CI has been upgraded to CI should be happy with v3 lockfiles.

@aaditmshah aaditmshah force-pushed the feature/fetch-json branch from cc70530 to f4d8221 Compare April 4, 2023 13:55
@uhyo
Copy link
Owner

uhyo commented Apr 5, 2023

CI was falling again so I pushed the result of npm install for fix. Does this look fine? 😇

@aaditmshah
Copy link
Contributor Author

CI was falling again so I pushed the result of npm install for fix. Does this look fine? 😇

Looks good to me.

Copy link
Owner

@uhyo uhyo left a comment

Choose a reason for hiding this comment

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

Awesome, thank you 😃

@uhyo
Copy link
Owner

uhyo commented Apr 5, 2023

I think I can release tomorrow 🙂

@uhyo uhyo merged commit 5cc0b10 into uhyo:master Apr 5, 2023
@aaditmshah aaditmshah deleted the feature/fetch-json branch April 5, 2023 12:34
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.

[feature request] stricter fetch().json()
2 participants