Skip to content

chore: optimize some interfaces & fix typo - #8634

Merged
ZxBing0066 merged 3 commits into
developfrom
chore/some-issues-when-familiarizing
Apr 23, 2025
Merged

chore: optimize some interfaces & fix typo#8634
ZxBing0066 merged 3 commits into
developfrom
chore/some-issues-when-familiarizing

Conversation

@ZxBing0066

Copy link
Copy Markdown
Member

Changes

  • Fix some typos
  • Code clean
  • Optimize some interface definition

@ZxBing0066 ZxBing0066 self-assigned this Apr 22, 2025
@ZxBing0066
ZxBing0066 marked this pull request as draft April 22, 2025 09:02
@@ -0,0 +1,7 @@
import { defineConfig } from 'vitest/config';

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

When I use the official Vitest VSCode extension, it doesn't list the tests in insomnia-scripting-environment, because it cannot find the config file. Add a simple one to solve this issue.

const modelOptionalKeys: string[] = 'optionalKeys' in model ? model.optionalKeys || [] : [];
// Prune extra keys from doc
for (const key of Object.keys(migratedDoc)) {
for (const key of typedKeys(migratedDoc)) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Use a helper to solve the type inference issue.


// @ts-expect-error -- TSCONVERSION not sure why this error is occurring
return migratedDoc;
return migratedDoc as T;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

as should be better than the magic comment.

};

export const cancellablePromise = ({ signal, fn }: { signal: AbortSignal; fn: Promise<any> }) => {
export const cancellablePromise = <T = any>({ signal, fn }: { signal: AbortSignal; fn: Promise<T> }) => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Enhance the type inference of cancellablePromise, now it can automatically get the return type from fn.

baseEnvironment: object;
cookieJar: CookieJar;
};
return result;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The result should be RequestContext, remove the unnecessary as.

@ZxBing0066
ZxBing0066 marked this pull request as ready for review April 22, 2025 09:22
@ZxBing0066
ZxBing0066 requested a review from ihexxa April 22, 2025 09:22
ihexxa
ihexxa previously approved these changes Apr 22, 2025

@ihexxa ihexxa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good job and thanks for sorting out these 👍.

return { error: `Script executor returns error: ${originalOutput.error}` };
}
const output = originalOutput as RequestContext;
const output = originalOutput;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: probably we can just name originalOutput as output at line 438.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ihexxa Good catch! I've renamed it, please have another review. 🙏

@ihexxa ihexxa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀

@ZxBing0066
ZxBing0066 merged commit d5b9952 into develop Apr 23, 2025
@ZxBing0066
ZxBing0066 deleted the chore/some-issues-when-familiarizing branch April 23, 2025 03:47
cwangsmv pushed a commit that referenced this pull request Apr 25, 2025
- Fix some typos
- Code clean
- Optimize some interface definition
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