-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: support specific importer when import from curl #9197
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the curl import functionality by adding support for specifying a particular importer and properly handling empty states. The changes ensure that when importing from curl, only the curl importer is used rather than attempting to match against all available importers.
- Modified the
convertfunction to accept an optionalimporterIdparameter to filter importers - Updated the PasteCurlModal to clear state when input is empty and specify the curl importer
- Enhanced input validation by resetting validation state and request object when the input value is cleared
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/insomnia/src/utils/importers/convert.ts | Added optional importerId parameter to filter importers by ID |
| packages/insomnia/src/ui/components/modals/paste-curl-modal.tsx | Clear state on empty input and specify curl importer when converting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| insecureReadFileWithEncoding: options => ipcRenderer.invoke('insecureReadFileWithEncoding', options), | ||
| secureReadFile: options => ipcRenderer.invoke('secureReadFile', options), | ||
| parseImport: options => ipcRenderer.invoke('parseImport', options), | ||
| parseImport: (...args) => ipcRenderer.invoke('parseImport', ...args), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for looking at this bug could you share a repro. notice the structure in this file? @ZxBing0066
its a convention... happy to discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jackkav I use ...args here because parseImport needs more than one argument. If we want to use only the options, we need to declare more interfaces, more glue code, and it is easy to cause bugs since we need to transform the args.
With only options
interface BridgeAPI {
parseImport: ({
importEntry: ImportEntry,
options?: { importerId?: string }
}) => Return<typeof convert>
}
ipcMainHandle('parseImport', async (_, options: {
importEntry: ImportEntry,
options: { importerId?: string }
}) => {
return convert(options.importEntry, options.options);
});With ...args
interface BridgeAPI {
parseImport: typeof convert
}
ipcMainHandle('parseImport', async (_, ...args) => {
return convert(...args);
});Less code means fewer bugs and fewer dependencies. And it could avoid inconsistent calls the the same functionality. Like in the render process we need to call with one options, but in the main process, we need to call with multiple parameters. It looks like chaos.
And these codes are all boilerplate code, we can even simplify them to:
const BridgeAPIMap = {
parseImport,
writeFile,
};
export type BridgeAPI = typeof BridgeAPIMap;
for (const action in BridgeAPIMap) {
ipcMainHandle(action, async (_, ...args) => {
return BridgeAPIMap[action](...args);
});
}No need to declare them one by one. It's also easy to understand that all the interfaces are the same when we call them from the render process or from the main process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use ...args here because parseImport needs more than one argument.
thats exactly why we use options so its type safe
named arguments are safer than ordered arguments. since we didn't have a safe way to type it previously this is the compromise we landed on. If you have a suggestion to improve the convention I'm curious to hear about it and see a spike showing me how it works.
This is better discussed face to face where possible as I don't want there to be misunderstandings.
INS-1503
fixes #9151
Changes