fix(load): updated queue utils #12
Conversation
makhnatkin
force-pushed
the
fix-load-queue
branch
from
12adc67 to
95afacf
Compare
src/lib/extension-load-queue.ts
Outdated
| isQueueCreated?: boolean; | ||
|
|
||
| // Callback to handle queue creation |
There was a problem hiding this comment.
Interface members should be annotated with JSDoc-styled comments as well /** */
| if (isBrowser()) { | ||
| (window as Window)[key] = (window as Window)[key] || []; | ||
| return (window as Window)[key]; | ||
| // Initialize the store if it does not exist or check that it is an array |
There was a problem hiding this comment.
In my opinion, this commenting style is too verbose. JSDoc is fine, but I don't think we really need to comment on every other statement.
src/lib/extension-load-queue.ts
Outdated
| window[QUEUES_SYMBOL] = {}; | ||
| } | ||
| } else { | ||
| throw new Error('This functionality should be employed on the client-side.'); |
There was a problem hiding this comment.
I don't think this error message is self-descriptive enough. The message should mention that it comes from a load queue. Maybe something along the lines of Cannot initialize QueueStore in a non-browser environment?
src/lib/extension-load-queue.ts
Outdated
| store.splice(index, 1); | ||
| } | ||
| }; | ||
| } else { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('Store is not provided to useController'); // Replace console.warn with a logging function if necessary |
There was a problem hiding this comment.
I'd say just throwing makes more sense here, maybe even removing the if (store) check altogether, since store cannot be nullable judging from the argument types
src/lib/index.ts
Outdated
|
|
||
| export { | ||
| AttrsParser, | ||
| createLoadQueue, | ||
| getQueueStore, | ||
| getScriptStore, | ||
| handleQueueCreated, | ||
| createHandleQueueCreated, |
There was a problem hiding this comment.
Do we really need to export this? This seems like an internal handler.
| }: CreateLoadQueueArgs<T>) => { | ||
| queueKey = SINGLE_QUEUE_SYMBOL, | ||
| isQueueCreated = getQueueStore(queueKey), | ||
| onQueueCreated = createHandleQueueCreated(queueKey), |
There was a problem hiding this comment.
I can't say I like the fact we can pass in custom onQueueCreated, which would effectively break the whole system — the default handler does a very important job of tracking which queues were initialized. So my guess is we don't want to expose this option to the end user.
There was a problem hiding this comment.
I did not want to associate the createLoadQueue with the window runtime, assuming that the queue logic could generally work with any data source (store). I suggested leaving the prop onQueueCreated.
However, I agree that createHandleQueueCreated function could not be exported.
| /** | ||
| * The store where callbacks are saved. | ||
| * | ||
| * @type {ScriptStore<T>} |
There was a problem hiding this comment.
This is a nitpick, but I'd say it makes very little sense to use JSDoc type annotations in presence of TypeScript.
No description provided.