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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ef67c4
feat: enable TS strict mode
kanadgupta Sep 20, 2023
fe7a714
chore: more tsconfig refreshes
kanadgupta Sep 20, 2023
775769a
fix: bump target down
kanadgupta Sep 26, 2023
07e458c
chore: test type cleanup
kanadgupta Sep 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ function isFile(value: any) {
}

function getFileFromSuppliedFiles(filename: string, files: FetchHAROptions['files']) {
if (filename in files) {
if (files && filename in files) {
return files[filename];
} else if (decodeURIComponent(filename) in files) {
} else if (files && decodeURIComponent(filename) in files) {
return files[decodeURIComponent(filename)];
}

Expand Down Expand Up @@ -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!

if (!('mimeType' in request.postData)) {
// @ts-expect-error HAR spec requires that `mimeType` is always present but it might not be.
request.postData.mimeType = 'application/octet-stream';
Expand All @@ -163,7 +163,9 @@ export default function fetchHAR(har: Har, opts: FetchHAROptions = {}) {
headers.set('Content-Type', request.postData.mimeType);

const encodedParams = new URLSearchParams();
request.postData.params.forEach(param => encodedParams.set(param.name, param.value));
request.postData.params?.forEach(param => {
if (param.value) encodedParams.set(param.name, param.value);
});

options.body = encodedParams.toString();
break;
Expand All @@ -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

if ('fileName' in param && param.fileName) {
if (opts.files) {
const fileContents = getFileFromSuppliedFiles(param.fileName, opts.files);
if (fileContents) {
Expand All @@ -197,7 +199,7 @@ export default function fetchHAR(har: Har, opts: FetchHAROptions = {}) {
form.append(
param.name,
new File([fileContents], param.fileName, {
type: param.contentType || null,
type: param.contentType || undefined,
}),
param.fileName,
);
Expand All @@ -214,15 +216,15 @@ export default function fetchHAR(har: Har, opts: FetchHAROptions = {}) {
}
}

if ('value' in param) {
if ('value' in param && param.value) {
let paramBlob;
const parsed = parseDataUrl(param.value);
if (parsed) {
// If we were able to parse out this data URL we don't need to transform its data
// into a buffer for `Blob` because that supports data URLs already.
paramBlob = new Blob([param.value], { type: parsed.contentType || param.contentType || null });
paramBlob = new Blob([param.value], { type: parsed.contentType || param.contentType || undefined });
} else {
paramBlob = new Blob([param.value], { type: param.contentType || null });
paramBlob = new Blob([param.value], { type: param.contentType || undefined });
}

form.append(param.name, paramBlob, param.fileName);
Expand All @@ -234,17 +236,17 @@ export default function fetchHAR(har: Har, opts: FetchHAROptions = {}) {
);
}

form.append(param.name, param.value);
if (param.value) form.append(param.name, param.value);
});

options.body = form;
break;

default:
const formBody: Record<string, unknown> = {};
request.postData.params.map(param => {
request.postData.params?.map(param => {
try {
formBody[param.name] = JSON.parse(param.value);
formBody[param.name] = JSON.parse(param.value || '');
} catch (e) {
formBody[param.name] = param.value;
}
Expand All @@ -254,7 +256,7 @@ export default function fetchHAR(har: Har, opts: FetchHAROptions = {}) {

options.body = JSON.stringify(formBody);
}
} else if (request.postData.text?.length) {
} else if (request.postData?.text?.length) {
// If we've got `files` map content present, and this post data content contains a valid data
// URL then we can substitute the payload with that file instead of the using data URL.
if (opts.files) {
Expand Down
7 changes: 5 additions & 2 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

"noImplicitAny": true,
"outDir": "dist/"
"outDir": "dist/",
"strict": true,
"target": "ES2022"
kanadgupta marked this conversation as resolved.
Show resolved Hide resolved
},
"include": ["./src/**/*"]
}