-
Notifications
You must be signed in to change notification settings - Fork 6
fix: allow to run on windows without context isolation #19
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
base: main
Are you sure you want to change the base?
Conversation
improve the preloader to run also on windows without context isolation
felixrieseberg
left a comment
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.
Thank you, this looks good!
|
Please update the tests - once you do, I'm happy to merge it in! |
now all tests also test the case of no context isolated windows
felixrieseberg
left a comment
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 updating the tests! The actual code still looks good, but the tests probably need some work. I've left some comments!
| @@ -1,7 +1,11 @@ | |||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | |||
| import { loadElectronLlm } from '../src/preload/index.ts'; | |||
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.
Hm, with this change we're no longer testing the actual file!
| }); | ||
|
|
||
| // Create a clean copy of the electronAi API to test with | ||
| const createElectronAiApi = () => { |
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 think we could and should import the actual API we're trying to test!
| }; | ||
|
|
||
| // Create our own loadElectronLlm function for testing | ||
| const loadElectronLlmForTests = async (contextIsolated: boolean) => { |
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.
This seems to be the code we're actually trying to test, but a copy of it!
| await loadElectronLlm(); | ||
| ipcRenderer = (await import('electron')).ipcRenderer; | ||
| }); | ||
| // Tests with context isolation enabled |
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.
Most of these tests do the exact same thing as without context isolation, so we should probably condense them into just one group of tests!
| }); | ||
|
|
||
| await (globalThis as any).electronAi.promptStreaming(input); | ||
| it('should handle errors during API exposure', async () => { |
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.
This is great!
improve the preloader to run also on windows without context isolation