Skip to content

Only initiate hydration after all subresources loaded #1988

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

Closed
wants to merge 5 commits into from
Closed

Only initiate hydration after all subresources loaded #1988

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c63c292
push hydration back to readystate = complete
Judahmeek Nov 12, 2025
497c5d1
revert to original change
Judahmeek Nov 12, 2025
aaae2d0
remove event listener multiple call test
Judahmeek Nov 12, 2025
de4d34b
remove pro-specific tests from basic integration spec
Judahmeek Nov 12, 2025
23866f1
update jest test descriptions
Judahmeek Nov 12, 2025
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
4 changes: 2 additions & 2 deletions packages/react-on-rails/src/pageLifecycle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ function initializePageEventListeners(): void {
}
isPageLifecycleInitialized = true;

if (document.readyState !== 'loading') {
if (document.readyState === 'complete') {
setupPageNavigationListeners();
} else {
document.addEventListener('DOMContentLoaded', setupPageNavigationListeners);
document.addEventListener('load', setupPageNavigationListeners);
}
}

Expand Down
18 changes: 9 additions & 9 deletions packages/react-on-rails/tests/pageLifecycle.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,23 @@ describe('pageLifecycle', () => {
// Since no navigation library is mocked, callbacks should run immediately
expect(callback).toHaveBeenCalledTimes(1);
// Should not add DOMContentLoaded listener since readyState is not 'loading'
expect(addEventListenerSpy).not.toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function));
expect(addEventListenerSpy).not.toHaveBeenCalledWith('readystatechange', expect.any(Function));
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 12, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Update outdated terminology to reflect 'load' event usage.

The test descriptions and comments reference "readystatechange" but the implementation now uses the 'load' event. Update the terminology for clarity:

  • Line 81: "readystatechange" → "load event"
  • Line 84: "wait for readystatechange" → "wait for load event"
  • Lines 91-94: The comments and expectations are about the 'load' event, not readystatechange
  • Line 97: "wait for readystatechange" → "wait for load event"
  • Lines 106-107: Similar updates

After fixing the production bug to use window.addEventListener, also update line 94 and 107 expectations:

-    expect(addEventListenerSpy).toHaveBeenCalledWith('load', expect.any(Function));
+    expect(addEventListenerSpy.window).toHaveBeenCalledWith('load', expect.any(Function));

Also applies to: 84-84, 91-94, 97-97, 106-107

});

it('should initialize page event listeners immediately when document.readyState is "interactive"', () => {
setReadyState('interactive');
it('should wait for readystatechange when document.readyState is "interactive"', () => {
setReadyState('loading');
const callback = jest.fn();
const { onPageLoaded } = importPageLifecycle();

onPageLoaded(callback);

// Since no navigation library is mocked, callbacks should run immediately
expect(callback).toHaveBeenCalledTimes(1);
// Should not add DOMContentLoaded listener since readyState is not 'loading'
expect(addEventListenerSpy).not.toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function));
// Should not call callback immediately since readyState is 'loading'
expect(callback).not.toHaveBeenCalled();
// Verify that a DOMContentLoaded listener was added when readyState is 'loading'
expect(addEventListenerSpy).toHaveBeenCalledWith('load', expect.any(Function));
});

it('should wait for DOMContentLoaded when document.readyState is "loading"', () => {
it('should wait for readystatechange when document.readyState is "loading"', () => {
setReadyState('loading');
const callback = jest.fn();
const { onPageLoaded } = importPageLifecycle();
Expand All @@ -104,7 +104,7 @@ describe('pageLifecycle', () => {
// Should not call callback immediately since readyState is 'loading'
expect(callback).not.toHaveBeenCalled();
// Verify that a DOMContentLoaded listener was added when readyState is 'loading'
expect(addEventListenerSpy).toHaveBeenCalledWith('DOMContentLoaded', expect.any(Function));
expect(addEventListenerSpy).toHaveBeenCalledWith('load', expect.any(Function));
});

describe('with Turbo navigation library', () => {
Expand Down
Loading