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

feat: enable TS strict mode #425

Merged
merged 4 commits into from
Sep 27, 2023
Merged

feat: enable TS strict mode #425

merged 4 commits into from
Sep 27, 2023

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Sep 20, 2023

Changes

Enables TypeScript's strict mode on this codebase. Mostly just adding type-guards and the like! No real code changes as far as we should be concerned.

@kanadgupta kanadgupta added bug Something isn't working enhancement New feature or request labels Sep 20, 2023
@kanadgupta kanadgupta requested review from a team, erunion, Dashron, domharrington and mosesj-readme and removed request for a team and erunion September 26, 2023 17:51
tsconfig.json Outdated Show resolved Hide resolved
@@ -186,8 +188,8 @@ export default function fetchHAR(har: Har, opts: FetchHAROptions = {}) {

const form = new FormData();

request.postData.params.forEach(param => {
if ('fileName' in param) {
request.postData.params?.forEach(param => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might DRY things up a bit to deconstruct params up here

@@ -4,9 +4,12 @@
"baseUrl": "./src",
"declaration": true,
"esModuleInterop": true,
"lib": ["dom", "es2020"],
"lib": ["DOM", "ES2022"],
"module": "NodeNext",
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean it outputs ESM?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, we're doing that in #427! we're still emitting CJS in this PR, but this just gives us support for the exports object in the package.json when loading in external packages. you can read more at these links:

https://www.typescriptlang.org/tsconfig/#node16nodenext-nightly-builds

https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing

@kanadgupta kanadgupta merged commit 00a2d41 into main Sep 27, 2023
7 checks passed
@kanadgupta kanadgupta deleted the ts-strict-mode branch September 27, 2023 14:01
@@ -144,7 +144,7 @@ export default function fetchHAR(har: Har, opts: FetchHAROptions = {}) {
}

if ('postData' in request) {
if ('params' in request.postData) {
if (request.postData && 'params' in request.postData) {
Copy link
Member

Choose a reason for hiding this comment

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

Kinda funky that TS wants you to check request.postData again here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah 😬 I wonder there's some minor detail about in that TS knows that we don't know here lol

Copy link

Choose a reason for hiding this comment

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

ts won't remember existence checks because side effects could change the value. You would want to throw postData into a variable at line 145 and then check that at 146, and use it at 147.

Copy link
Member Author

Choose a reason for hiding this comment

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

ohhh that's a great callout @Dashron!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants