Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: allow dynamically import atmosphere.js for SW context #2901

Open
wants to merge 5 commits into
base: fix/sw-context-access
Choose a base branch
from
Open

fix: allow dynamically import atmosphere.js for SW context #2901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dfca9e8
Allow dynamically import atmosphere.js for SW context
krissvaa Nov 11, 2024
784cefe
Fix dynamic import of atmosphere
krissvaa Nov 13, 2024
62fa608
Fix dynamic import of atmosphere
krissvaa Nov 13, 2024
33b8cc7
Fix dynamic import of atmosphere
krissvaa Nov 13, 2024
3ba43b0
Fix dynamic import of atmosphere as top level await not allowed
krissvaa Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions packages/ts/frontend/src/FluxConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { ReactiveControllerHost } from '@lit/reactive-element';
import atmosphere from 'atmosphere.js';
import type Atmosphere from 'atmosphere.js';
import type { Subscription } from './Connect.js';
import { getCsrfTokenHeadersForEndpointRequest } from './CsrfUtils.js';
import {
Expand Down Expand Up @@ -71,6 +71,16 @@
reconnect?(): ActionOnLostSubscription | void;
};

let atmosphere: Atmosphere.Atmosphere | undefined;

(async () => {
if (!import.meta.env?.VITE_SW_CONTEXT) {
atmosphere = await import('atmosphere.js').then((module) => module.default);
}
})().catch((e) => {
console.error('Failed to load atmosphere.js', e);

Check warning on line 81 in packages/ts/frontend/src/FluxConnection.ts

View check run for this annotation

Codecov / codecov/patch

packages/ts/frontend/src/FluxConnection.ts#L81 

Added line #L81 was not covered by tests
});
Comment on lines +76 to +82
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(async () => {
if (!import.meta.env?.VITE_SW_CONTEXT) {
atmosphere = await import('atmosphere.js').then((module) => module.default);
}
})().catch((e) => {
console.error('Failed to load atmosphere.js', e);
});
if (!import.meta.env?.VITE_SW_CONTEXT) {
try {
atmosphere = await import('atmosphere.js').then((module) => module.default);
} catch (e: unknown) {
console.error('Failed to load atmosphere.js', e);
}
}

I'd suggest using top-level await here. Is it possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we can probably check the presence of the document in globalThis here, instead of VITE_SW_CONTEXT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

top-level await is not allowed according to our project configuration.

Top-level await is not available in the configured target environment ("chrome87", "edge88", "es2020", "firefox78", "safari14" + 2 overrides)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid top-level await for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that creates a possible race condition, and in some cases the FluxConnection can possibly be created before the Atmosphere is loaded.


/**
* A representation of the underlying persistent network connection used for subscribing to Flux type endpoint methods.
*/
Expand Down Expand Up @@ -186,7 +196,7 @@
const extraHeaders = self.document ? getCsrfTokenHeadersForEndpointRequest(self.document) : {};
const pushUrl = 'HILLA/push';
const url = prefix.length === 0 ? pushUrl : (prefix.endsWith('/') ? prefix : `${prefix}/`) + pushUrl;
this.#socket = atmosphere.subscribe?.({
this.#socket = atmosphere?.subscribe?.({
contentType: 'application/json; charset=UTF-8',
enableProtocol: true,
transport: 'websocket',
Expand Down
10 changes: 10 additions & 0 deletions packages/ts/frontend/src/vite-env.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// / <reference types="vite/client" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// / <reference types="vite/client" />
/// <reference types="vite/client" />


// eslint-disable-next-line import/unambiguous
interface ImportMetaEnv {
readonly VITE_SW_CONTEXT?: boolean;
}

interface ImportMeta {
readonly env?: ImportMetaEnv;
}
Loading