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

TS Conversion > src/query-helpers.js #1009

Closed
wants to merge 2 commits into from

Conversation

riotrah
Copy link
Contributor

@riotrah riotrah commented Aug 18, 2021

What: Convert src/query-helpers.js to TS. Includes test snapshot updates, some type declaration file changes.

Why: Motivations are outlined in #494

How: Filename changes first. Then error fixes. Then fixes to make tests pass. I also occasionally changed files imported from or exported into from query-helpers to include pending changes made from other PRs that made TS changes, just for sanity checks and guiding inspiration whilst working on my changes. Of course, those changes were not included in this PR, nor were they kept around when finally running tests for my changes.

Checklist:

  • Documentation added to the
    docs site NA
  • Tests NA
  • TypeScript definitions updated
  • Ready to be merged

No, as I need feedback for the following concerns:

  1. There are some sections of code that raise type errors or lint errors, whose solution I cannot tell with what little I know of this library and its usage. Those sections require input from other community members/maintainers, and I've marked them with // FIXME: comments. I'll open threads (if possible, have not used GitHub in this manner before) for review at each point.

  2. There are some potentially questionable type definitions I've made where I've tried to square the existing types in types/query-helpers.d.ts with the errors raised by inlining those types into src/query-helpers.ts. Tests pass without too much finagling with // @ts-expect-error or // eslint-disable, yet, for the same reason as the previous point, I cannot fully tell if these are acceptable changes/compromises.

  3. Similar question to my other TS PRs (chore: Convert screen.js to TypeScript #1002 TS Conversion > src/events.js + edit src/events.d.ts #1008): I've changed public facing type declarations, are these changes acceptable?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 18, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@eps1lon
Copy link
Member

eps1lon commented Sep 7, 2021

Thanks for the work. I've convered the file in #1016 which was less ambitious to ensure files are considered as moved in GitHub.

@eps1lon eps1lon closed this Sep 7, 2021
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.

2 participants