[Feat] ZOOM Platform runner/service implementation #4882
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
| const zoomLibraryStore = new CacheStore<GameInfo[], 'games'>('zoom_library', null) // Added zoomLibraryStore | ||
| const zoomInstalledGamesStore = new TypeCheckedStoreFrontend( // Added zoomInstalledGamesStore | ||
| 'zoomInstalledGamesStore', | ||
| { | ||
| cwd: 'zoom_store', | ||
| name: 'installed' | ||
| } | ||
| ) | ||
| const zoomConfigStore = new TypeCheckedStoreFrontend('zoomConfigStore', { // Added zoomConfigStore | ||
| cwd: 'zoom_store' | ||
| }) | ||
|
|
There was a problem hiding this comment.
The Frontend directly messing with Electron Stores is a practice I'd like to avoid in the future if possible. Ideally use a Zustand store & push updates from the Backend
There was a problem hiding this comment.
Do you have a good example of how the code could look like from other stores? (or even from other codebases)
|
I'm also unsure how to deal with the linting. The CI output shows a large number of things to fix, but in code that was never modified in this PR. How to restrict the output to only the issues that are related with the code modified? |
you only need to worry about the errors, not the warnings, like: there are 12 errors and those are part of this PR |
|
You can also run |
|
CI is finally passing 🎉 . This PR still need a good amount of work, but this is the right track already. Thanks for your feedback and comments. 🙏 |
| nile: 'Amazon Games', | ||
| sideload: 'Other' | ||
| sideload: 'Other', | ||
| zoom: 'Zoom' |
There was a problem hiding this comment.
I think references to the store should be "ZOOM Platform" or even "zp" if a short string is needed not "Zoom" as that's simply wrong.
Not sure if classes/variables and stuff should be changed too.
src/backend/utils.ts
Outdated
| export function openAuthWindow( | ||
| url: string, | ||
| successUrl: string, | ||
| callback: (url: string) => Promise<{ status: 'done' | 'error' }> | ||
| ): Promise<{ status: 'done' | 'error' }> { | ||
| return new Promise((resolve) => { | ||
| let resolved = false | ||
| const authWindow = new BrowserWindow({ | ||
| width: 800, | ||
| height: 600, | ||
| show: false, | ||
| webPreferences: { | ||
| nodeIntegration: false, | ||
| contextIsolation: true | ||
| } | ||
| }) | ||
|
|
||
| authWindow.on('closed', () => { | ||
| if (!resolved) { | ||
| resolved = true | ||
| resolve({ status: 'error' }) | ||
| } | ||
| }) | ||
|
|
||
| authWindow.webContents.on('did-navigate', async (event, newUrl) => { | ||
| if (newUrl.startsWith(successUrl)) { | ||
| const result = await callback(newUrl) | ||
| if (result.status === 'done') { | ||
| if (!resolved) { | ||
| resolved = true | ||
| resolve(result) | ||
| } | ||
| authWindow.close() | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| authWindow.loadURL(url) | ||
| authWindow.show() | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Lets avoid doing separate windows for auth. We already have auth flow ready WebView component, which it seems like you already adjusted
There was a problem hiding this comment.
Can you take a look to see if the new code makes sense? It a bit buggy but at least wanted to know if it is going into the right direction.
|
Reviews and comments even at this point are very appreciated, since this is my first PR to Heroic (and a large one!) |
neuromancer
changed the title
Co-authored-by: Paweł Lidwin <[email protected]>
Co-authored-by: Paweł Lidwin <[email protected]>
|
Can I get a second round of reviews and comments please? 😇 |
|
Should I add specific tests and documentation for this? |
6 participants
This PR brings the ZOOM platform to Heroic.
The code is based on the corresponding Lutris PR, which is known to work correctly. Please help me providing feedback and flagging unrelated changed as this was heavily assisted by Gemini taking the Lutris implementation as a base (which I developed personally).
Checklist