-
-
Notifications
You must be signed in to change notification settings - Fork 635
Fix initial page startup race for late-loading client bundles #3151
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
base: main
Are you sure you want to change the base?
Changes from all commits
6e02cf7
219545c
6458d09
05e43fb
3c3ebcf
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 |
|---|---|---|
|
|
@@ -31,6 +31,9 @@ describe('pageLifecycle', () => { | |
| // eslint-disable-next-line global-require | ||
| const importPageLifecycle = () => require('../src/pageLifecycle.ts'); | ||
|
|
||
| const getEventHandler = (eventName) => | ||
| addEventListenerSpy.mock.calls.find((call) => call[0] === eventName)?.[1]; | ||
|
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.
|
||
|
|
||
| // Helper function to create navigation library mock | ||
| const createNavigationMock = (overrides = {}) => ({ | ||
| debugTurbolinks: jest.fn(), | ||
|
|
@@ -81,7 +84,7 @@ describe('pageLifecycle', () => { | |
| expect(addEventListenerSpy).not.toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function)); | ||
| }); | ||
|
|
||
| it('should wait for DOMContentLoaded when when document.readyState is "interactive"', () => { | ||
| it('should wait for DOMContentLoaded when document.readyState is "interactive"', () => { | ||
| setReadyState('interactive'); | ||
| const callback = jest.fn(); | ||
| const { onPageLoaded } = importPageLifecycle(); | ||
|
|
@@ -92,6 +95,7 @@ describe('pageLifecycle', () => { | |
| expect(callback).not.toHaveBeenCalled(); | ||
| // Verify that a DOMContentLoaded listener was added when readyState is 'interactive' | ||
| expect(addEventListenerSpy).toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function)); | ||
| expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function)); | ||
|
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. Good addition here. The symmetric negative assertion is missing from the // in the 'loading' test:
expect(addEventListenerSpy).not.toHaveBeenCalledWith('readystatechange', expect.any(Function));Since
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. Good — this assertion documents that // in the 'loading' state test, after the existing DOMContentLoaded assertion:
expect(addEventListenerSpy).not.toHaveBeenCalledWith('readystatechange', expect.any(Function));This documents the intentional asymmetry: the fallback listener is unnecessary when |
||
| }); | ||
|
|
||
| it('should wait for DOMContentLoaded when document.readyState is "loading"', () => { | ||
|
|
@@ -272,8 +276,10 @@ describe('pageLifecycle', () => { | |
| // - It tries to hydrate components BEFORE step 5 completes | ||
| // - ComponentRegistry.get() throws "Could not find component registered with name" | ||
| // | ||
| // The fix: Use `readyState === 'complete'` to ensure we wait for DOMContentLoaded | ||
| // (which fires AFTER deferred scripts execute) | ||
| // The fix: do not initialize immediately during `interactive`. | ||
| // Wait for DOMContentLoaded (which fires AFTER deferred scripts execute), | ||
| // and separately recover on the later `complete` transition only if | ||
| // DOMContentLoaded was already missed before startup ran. | ||
|
|
||
| it('should NOT call callbacks immediately when readyState is "interactive" because deferred scripts may not have executed', () => { | ||
| // Simulate the state right after HTML parsing completes but before deferred scripts run | ||
|
|
@@ -285,9 +291,9 @@ describe('pageLifecycle', () => { | |
| const hydrateComponentCallback = jest.fn(); | ||
| onPageLoaded(hydrateComponentCallback); | ||
|
|
||
| // CRITICAL: With the correct implementation (readyState === 'complete'), | ||
| // the callback should NOT be called immediately when readyState is 'interactive'. | ||
| // This gives deferred scripts time to execute and register components. | ||
| // CRITICAL: With the correct implementation, the callback should NOT be | ||
| // called immediately when readyState is 'interactive'. This gives deferred | ||
| // scripts time to execute and register components before hydration runs. | ||
| expect(hydrateComponentCallback).not.toHaveBeenCalled(); | ||
|
|
||
| // Instead, a DOMContentLoaded listener should be added | ||
|
|
@@ -324,7 +330,7 @@ describe('pageLifecycle', () => { | |
|
|
||
| onPageLoaded(attemptHydration); | ||
|
|
||
| // With correct implementation: callback not called yet, so no error | ||
| // With the correct implementation: callback not called yet, so no error | ||
| expect(attemptHydration).not.toHaveBeenCalled(); | ||
|
|
||
| // Simulate deferred script executing (this happens before DOMContentLoaded) | ||
|
|
@@ -339,12 +345,12 @@ describe('pageLifecycle', () => { | |
|
|
||
| // Now when DOMContentLoaded fires, the component is registered | ||
| // and hydration succeeds | ||
| domContentLoadedHandler(); | ||
| domContentLoadedHandler(new Event('DOMContentLoaded')); | ||
| expect(attemptHydration).toHaveBeenCalled(); | ||
| // No error thrown because componentRegistered is now true | ||
| }); | ||
|
|
||
| it('should handle the case where readyState transitions from interactive to complete', () => { | ||
| it('should recover when DOMContentLoaded was already missed during interactive by waiting for complete', () => { | ||
| // Start in 'interactive' state (HTML parsed, deferred scripts may be running) | ||
| setReadyState('interactive'); | ||
|
|
||
|
|
@@ -356,18 +362,38 @@ describe('pageLifecycle', () => { | |
| // Callback should not be called immediately at 'interactive' | ||
| expect(callback).not.toHaveBeenCalled(); | ||
| expect(addEventListenerSpy).toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function)); | ||
| expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function)); | ||
|
|
||
| // Get the DOMContentLoaded handler | ||
| const domContentLoadedHandler = addEventListenerSpy.mock.calls.find( | ||
| (call) => call[0] === 'DOMContentLoaded', | ||
| )?.[1]; | ||
| // Simulate the case where DOMContentLoaded already fired before startup installed its listener. | ||
| // The later transition to complete should still recover initialization. | ||
| const readyStateChangeHandler = getEventHandler('readystatechange'); | ||
| expect(readyStateChangeHandler).toBeDefined(); | ||
|
|
||
| // Simulate state transition: deferred scripts complete, then DOMContentLoaded fires | ||
| setReadyState('complete'); | ||
| domContentLoadedHandler(); | ||
| readyStateChangeHandler(new Event('readystatechange')); | ||
|
|
||
| // Now callback should have been called | ||
| // Now callback should have been called through the complete fallback | ||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('should remove both listeners after DOMContentLoaded fires so a later complete cannot re-trigger', () => { | ||
| setReadyState('interactive'); | ||
|
|
||
| const { onPageLoaded } = importPageLifecycle(); | ||
| const callback = jest.fn(); | ||
|
|
||
| onPageLoaded(callback); | ||
|
|
||
| const domContentLoadedHandler = getEventHandler('DOMContentLoaded'); | ||
| expect(domContentLoadedHandler).toBeDefined(); | ||
|
|
||
| domContentLoadedHandler(new Event('DOMContentLoaded')); | ||
| expect(callback).toHaveBeenCalledTimes(1); | ||
|
|
||
| // The handler's cleanup is what prevents the later readystatechange→complete | ||
| // event from firing the callback a second time in real browsers. | ||
| expect(removeEventListenerSpy).toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function)); | ||
| expect(removeEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function)); | ||
| }); | ||
|
Comment on lines
+379
to
+397
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. This test verifies that A stronger version would simulate firing it('should remove both listeners after DOMContentLoaded fires so a later complete cannot re-trigger', () => {
setReadyState('interactive');
const { onPageLoaded } = importPageLifecycle();
const callback = jest.fn();
onPageLoaded(callback);
// Restore real removeEventListener so cleanup actually works
removeEventListenerSpy.mockRestore();
const domContentLoadedHandler = getEventHandler('DOMContentLoaded');
const readystateChangeHandler = getEventHandler('readystatechange');
domContentLoadedHandler(new Event('DOMContentLoaded'));
expect(callback).toHaveBeenCalledTimes(1);
// Simulate the 'complete' transition that must not re-trigger
setReadyState('complete');
readystateChangeHandler(new Event('readystatechange'));
expect(callback).toHaveBeenCalledTimes(1); // still 1, not 2
}); |
||
| }); | ||
| }); | ||
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.
The idempotency here relies entirely on the two
removeEventListenercalls above succeeding before either browser event can re-enter this handler. That's safe in practice (single-threaded JS, well-behaved browsers), butsetupPageNavigationListenersitself has no guard — if it were ever called a second time it would attach duplicateturbo:render/turbolinks:renderlisteners and double-fire every subsequent page-loaded callback.Consider adding a one-line guard at the top of
setupPageNavigationListeners:This makes the defence explicit rather than implicit.