feat: added extension-load-queue #8

makhnatkin · 2024-09-01T16:53:23Z

The mermaid, latex and HTML packages use the mechanism of asynchronous loading of runtime scripts. This process is implemented via a queue and by adding a field to the window object. This PR code is incorporated into a set of functions that can be used in any Diplodoc extension package.To protect against external access, the Symbol data type has been used instead of a string. The code has already been tested and debugged in the HTML extension

Because the .eslintrc.js file is generated using a pre-commit hook and the lint, typecheck and build scripts requires different TS configurations, a separate TS configuration file (tsconfig.build.json) has been created.

makhnatkin · 2024-09-02T10:21:05Z

.eslintrc.js

@@ -3,6 +3,6 @@ module.exports = {
    extends: require.resolve('@diplodoc/lint/eslint-config'),
    parserOptions: {
        tsconfigRootDir: __dirname,
-        project: ['./tsconfig.json', './tsconfig.tests.json'],


This code will be deleted every time by a pre-commit hook

yes, I will change it in @diplodoc/lint package.

In this PR, I went another way - I created an alternative TS configuration for the build.

brotheroftux · 2024-09-02T12:14:36Z

scripts/bundle.js

        nodeExternalsPlugin(),
    ],
 });
+
+build({


TBH, I feel like making a clear distinction between react and non-react parts of code in this package is a particularly debatable decision. I'd vouch for just keeping it simple — that way, ELQ's react and non-react parts should probably reside in the same folder.

Also, while this is out of scope for this PR, I'd also argue that using ESBuild in the case of this particular package is also a pretty uncanny decision; in my opinion, it's better to ship all utility packages as a non-bundled set of TypeScript sources, tree shaking ought to do quite literally everything else.

TBH, I feel like making a clear distinction between react and non-react parts of code in this package is a particularly debatable decision. I'd vouch for just keeping it simple — that way, ELQ's react and non-react parts should probably reside in the same folder.

I thought like this: hooks for React require the import of a React package. At the same time, the rest of the utilities seem to be more like a set of simple functions. I would like to be able to use non-React utilities separately. Additionally, it seems to me that other packages in diplodoc-platform are structured in a similar manner. But I don't have a strong opinion here.

Also, while this is out of scope for this PR, I'd also argue that using ESBuild in the case of this particular package is also a pretty uncanny decision; in my opinion, it's better to ship all utility packages as a non-bundled set of TypeScript sources, tree shaking ought to do quite literally everything else.

Interesting thoughts. I suggest solving them separately, not within the framework of this PR.

brotheroftux · 2024-09-02T12:15:19Z

src/lib/extension-load-queue.ts

+
+export type ControllerLoadedCallback<T> = (controller: T) => void;
+
+export const QUEUE_SYMBOL: unique symbol = Symbol.for('queue');


No need to specify unique symbol explicitly, the Symbol.for statement is already inferred this way

brotheroftux · 2024-09-02T12:16:17Z

src/lib/extension-load-queue.ts

@@ -0,0 +1,86 @@
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+type BrowserWindow = any;


I'd rather use declaration merging here in lieu of specifying any explicitly.

brotheroftux · 2024-09-02T12:18:59Z

src/lib/extension-load-queue.ts

+    onQueueCreated?: (created: boolean) => void;
+}
+
+export const isBrowser = () => typeof window !== 'undefined' && typeof document !== 'undefined';


As I imagine, isBrowser should be a very frequently used utility, so if we're at all serious about this package and its usage, i'd place this in a separate top-level file and would make sure to document this.

I agree, at the same time, the current version implies use in place - I'll leave it like that for now, or rename it. I'll also add a comment about correcting it in the future.

brotheroftux · 2024-09-02T12:20:27Z

src/lib/extension-load-queue.ts

+
+export const QUEUE_SYMBOL: unique symbol = Symbol.for('queue');
+
+export type ScriptStore<T> = ControllerLoadedCallback<T>[] | null;


Wouldn't throwing an error ("This should only be used client-side") make more sense here though? In particular, it would allow to keep the API a bit more consistent without that null

brotheroftux · 2024-09-02T12:22:17Z

src/react/extension-load-queue.ts

+        if (store) {
+            store.push(setController);
+
+            return () => {


Honestly, I feel like it might be a better idea to auto-cleanup already fulfilled controller requests when processing the queue. Up for debate, though.

Up for debate, though.

That's right. For now, I just want to put the general code into the utils.

brotheroftux · 2024-09-03T13:12:45Z

.eslintignore

@@ -5,6 +5,7 @@
 .DS_Store
 node_modules
 /lib
+/react


This is extra at this point

brotheroftux · 2024-09-03T13:14:45Z

src/lib/extension-load-queue.ts

+declare global {
+    interface Window {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        [key: symbol]: any;


This is still sub-optimal, even though I won't mind as much if you decide to leave this as is. An optimal typing strategy would be something like https://github.com/diplodoc-platform/html-extension/blob/dea1790907586c976d0ec3cf1a7f8dae7f1859ea/src/types.ts#L28

In the example given, GLOBAL_SYMBOL is a pre-known constant. In mine, it is a key that is not known in advance. For the same reason, ScriptStore is generic in this PR. I propose to solve the problem of improved typing through future PR.

brotheroftux · 2024-09-03T13:15:34Z

src/lib/extension-load-queue.ts

+
+export const getQueueStore = () => {
+    if (isBrowser()) {
+        (window as Window)[QUEUE_SYMBOL] = (window as Window)[QUEUE_SYMBOL] || false;


as Window casts are weird — did you encounter any issues with declaration merging?

Yes, unfortunately, I still face a set of problems. I can't get rid of this cast. I propose to solve the problem of improved typing through future PR.

brotheroftux · 2024-09-03T13:17:16Z

src/lib/index.ts

@@ -0,0 +1,24 @@
+import {AttrsParser} from './attrs';


Feels like this file and extension-load-queue.ts should be put in their own respective directory.
Then you could use export * from './extension-load-queue'; in this file, which would be much tidier if more utilities are added.

feat: added extension-load-queue

62cbeb3

makhnatkin requested review from 3y3, d3m1d0v, v8tenko and brotheroftux September 2, 2024 08:10

makhnatkin marked this pull request as ready for review September 2, 2024 08:11

makhnatkin self-assigned this Sep 2, 2024

makhnatkin commented Sep 2, 2024

View reviewed changes

d3m1d0v approved these changes Sep 2, 2024

View reviewed changes

brotheroftux requested changes Sep 2, 2024

View reviewed changes

feat: updated build and types

ef95b14

makhnatkin requested a review from brotheroftux September 3, 2024 11:04

brotheroftux approved these changes Sep 3, 2024

View reviewed changes

feat: added TODO

36e3d68

makhnatkin merged commit f790e6a into master Sep 3, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added extension-load-queue #8

feat: added extension-load-queue #8

makhnatkin commented Sep 1, 2024 •

edited

Loading

makhnatkin Sep 2, 2024

v8tenko Sep 3, 2024

makhnatkin Sep 3, 2024

brotheroftux Sep 2, 2024

makhnatkin Sep 2, 2024

brotheroftux Sep 2, 2024

brotheroftux Sep 2, 2024

brotheroftux Sep 2, 2024

makhnatkin Sep 2, 2024

brotheroftux Sep 2, 2024

brotheroftux Sep 2, 2024

makhnatkin Sep 2, 2024 •

edited

Loading

brotheroftux Sep 3, 2024

brotheroftux Sep 3, 2024

makhnatkin Sep 3, 2024 •

edited

Loading

brotheroftux Sep 3, 2024

makhnatkin Sep 3, 2024 •

edited

Loading

brotheroftux Sep 3, 2024


		export type ControllerLoadedCallback<T> = (controller: T) => void;

		export const QUEUE_SYMBOL: unique symbol = Symbol.for('queue');

		@@ -0,0 +1,86 @@
		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		type BrowserWindow = any;


		export const QUEUE_SYMBOL: unique symbol = Symbol.for('queue');

		export type ScriptStore<T> = ControllerLoadedCallback<T>[] \| null;

feat: added extension-load-queue #8

feat: added extension-load-queue #8

Conversation

makhnatkin commented Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makhnatkin Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makhnatkin Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makhnatkin Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makhnatkin commented Sep 1, 2024 •

edited

Loading

makhnatkin Sep 2, 2024 •

edited

Loading

makhnatkin Sep 3, 2024 •

edited

Loading

makhnatkin Sep 3, 2024 •

edited

Loading