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

fix(types): widen too-narrow types #102

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

boneskull
Copy link
Collaborator

In #99 I changed stuff, but I made a mistake or two. This fixes those mistakes.

Additionally, added type coverage for tests.

@boneskull boneskull added the bug Something isn't working label Oct 31, 2023
@boneskull boneskull self-assigned this Oct 31, 2023
@boneskull boneskull force-pushed the boneskull/ts-fixes branch 3 times, most recently from 93cea1c to 7a2b5d7 Compare October 31, 2023 22:01
@boneskull boneskull requested a review from nzakas October 31, 2023 23:53
src/env.js Outdated Show resolved Hide resolved
@boneskull boneskull force-pushed the boneskull/ts-fixes branch 2 times, most recently from db56be4 to c5de5ff Compare November 1, 2023 18:42
@nzakas
Copy link
Contributor

nzakas commented Jan 9, 2024

@boneskull I think you still were fixing some things on this? Any other changes before we're ready to merge?

@boneskull
Copy link
Collaborator Author

@nzakas Ah, uh. Let me check. 😄

Since many functions accept `number`, which is then coerced to `string`, we need to account for that.

`Record<string, string>` for the type of `T` _would_ be fine, except:

1. `ProcessEnv` is `Record<string, undefined>`.  I'm guessing since you can write `process.env.cows = undefined`.  However, everything coming out of the actual environment in `process.env` is a `string`.
2. An empty object literal is not a `Record`, but it is an `object`. So we have to loosen the type arg `T` to extend `object`.
- Renamed the existing fixture to `ts-project-module-nodenext`
- Added a new fixture `ts-project-module-commonjs` which is identical except it uses the `commonjs` `module` option and thus `node` `moduleResolution`
@boneskull
Copy link
Collaborator Author

@nzakas OK, I'm done.

Copy link
Contributor

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@nzakas nzakas merged commit 6ad2e51 into humanwhocodes:main Jan 10, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants