-
Notifications
You must be signed in to change notification settings - Fork 511
refactor: parallelize bootstrap and simplify lifecycle with VueUse #8307
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
Changes from 3 commits
8afb45d
33c457a
326154f
0a73c5a
3c0b227
80d58dd
52483b3
51bce92
c171227
35eb028
16b285a
0ca910a
07e6daa
b11eb7e
69efc65
d09f948
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,9 @@ export const useNodeBadge = () => { | |
| } | ||
|
|
||
| onMounted(() => { | ||
| if (extensionStore.isExtensionInstalled('Comfy.NodeBadge')) return | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
|
||
| // TODO: Fix the composables and watchers being setup in onMounted | ||
| const nodePricing = useNodePricing() | ||
|
|
||
| watch( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -170,7 +170,11 @@ export const useSettingStore = defineStore('setting', () => { | |
| throw new Error('Settings must have an ID') | ||
| } | ||
| if (setting.id in settingsById.value) { | ||
| throw new Error(`Setting ${setting.id} must have a unique ID.`) | ||
| // Setting already registered - skip to allow component remounting | ||
| // TODO: Add store reset methods to bootstrapStore and settingStore, then | ||
| // replace window.location.reload() with router.push() in SidebarLogoutIcon.vue | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, yeah, this. (My comment above)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add this aspect to the comment/todo? Or make an Issue?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai Open a tracking issue to fix the state such that we can use the router to login/logout
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 57
🧬 Code Graph Analysis Results[src/platform/settings/settingStore.ts] (snippet inside addSetting)
Snippet: settingsById.value[setting.id] = setting if (settingValues.value[setting.id] !== undefined) { Notes:
🧠 Learnings used |
||
| console.warn(`Setting already registered: ${setting.id}`) | ||
| return | ||
DrJKL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| settingsById.value[setting.id] = setting | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import { createPinia, setActivePinia } from 'pinia' | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import { useBootstrapStore } from './bootstrapStore' | ||
|
|
||
| vi.mock('@/scripts/api', () => ({ | ||
| api: { | ||
| init: vi.fn().mockResolvedValue(undefined), | ||
| getNodeDefs: vi.fn().mockResolvedValue({ TestNode: { name: 'TestNode' } }), | ||
| getCustomNodesI18n: vi.fn().mockResolvedValue({}), | ||
| getUserConfig: vi.fn().mockResolvedValue({}) | ||
| } | ||
| })) | ||
|
|
||
| vi.mock('@/i18n', () => ({ | ||
| mergeCustomNodesI18n: vi.fn() | ||
| })) | ||
|
|
||
| vi.mock('@/platform/settings/settingStore', () => ({ | ||
| useSettingStore: vi.fn(() => ({ | ||
| loadSettingValues: vi.fn().mockResolvedValue(undefined) | ||
| })) | ||
| })) | ||
|
|
||
| vi.mock('@/stores/workspaceStore', () => ({ | ||
| useWorkspaceStore: vi.fn(() => ({ | ||
| workflow: { | ||
| syncWorkflows: vi.fn().mockResolvedValue(undefined) | ||
| } | ||
| })) | ||
| })) | ||
DrJKL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| describe('bootstrapStore', () => { | ||
| let store: ReturnType<typeof useBootstrapStore> | ||
|
|
||
| beforeEach(() => { | ||
| setActivePinia(createPinia()) | ||
| store = useBootstrapStore() | ||
| vi.clearAllMocks() | ||
| }) | ||
|
|
||
| it('initializes with all flags false', () => { | ||
| expect(store.isNodeDefsReady).toBe(false) | ||
| expect(store.isSettingsReady).toBe(false) | ||
| expect(store.isI18nReady).toBe(false) | ||
| }) | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| it('starts early bootstrap (node defs)', async () => { | ||
| const { api } = await import('@/scripts/api') | ||
|
|
||
| store.startEarlyBootstrap() | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect(store.isNodeDefsReady).toBe(true) | ||
| }) | ||
|
|
||
| expect(api.getNodeDefs).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('starts store bootstrap (settings, i18n)', async () => { | ||
| void store.startStoreBootstrap() | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect(store.isSettingsReady).toBe(true) | ||
| expect(store.isI18nReady).toBe(true) | ||
| }) | ||
| }) | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
DrJKL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }) | ||
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.
Should this logic/nuance be abstracted and exported from the store? So it's used like
await untilSettingsReady()? To be fair it's already quite expressive.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.
Yes. I considered doing that for each of the lazily initialized stores instead of keeping the logic in bootstrap. I'll try it out.
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.
Just lacking an analogous store for i18n pieces.