Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
24 changes: 13 additions & 11 deletions packages/react-on-rails/src/pageLifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,20 @@ function setupPageNavigationListeners(): void {
}

let isPageLifecycleInitialized = false;
function initializePageEventListeners(): void {
function onPageReady(callback: () => void) {
if (typeof window === 'undefined') return;

if (isPageLifecycleInitialized) {
return;
}
isPageLifecycleInitialized = true;

if (document.readyState !== 'loading') {
setupPageNavigationListeners();
if (document.readyState === 'complete') {
if (isPageLifecycleInitialized) {
return;
}
isPageLifecycleInitialized = true;
callback();
} else {
document.addEventListener('DOMContentLoaded', setupPageNavigationListeners);
document.addEventListener('readystatechange', function onReadyStateChange() {
document.removeEventListener('readystatechange', onReadyStateChange);
onPageReady(callback);
});
}
}

Expand All @@ -79,13 +81,13 @@ export function onPageLoaded(callback: PageLifecycleCallback): void {
void callback();
}
pageLoadedCallbacks.add(callback);
initializePageEventListeners();
onPageReady(setupPageNavigationListeners);
}

export function onPageUnloaded(callback: PageLifecycleCallback): void {
if (currentPageState === 'unload') {
void callback();
}
pageUnloadedCallbacks.add(callback);
initializePageEventListeners();
onPageReady(setupPageNavigationListeners);
}
39 changes: 9 additions & 30 deletions packages/react-on-rails/tests/pageLifecycle.test.js
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

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('readystatechange', 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('readystatechange', expect.any(Function));
});

describe('with Turbo navigation library', () => {
Expand Down Expand Up @@ -232,25 +232,4 @@ describe('pageLifecycle', () => {
global.window = originalWindow;
});
});

describe('preventing duplicate initialization', () => {
it('should not initialize listeners multiple times', () => {
setReadyState('loading');
const { onPageLoaded } = importPageLifecycle();
const callback1 = jest.fn();
const callback2 = jest.fn();

// First call should initialize and call addEventListener
onPageLoaded(callback1);
expect(addEventListenerSpy).toHaveBeenCalledTimes(1);

// Second call should not add more listeners (isPageLifecycleInitialized is true)
onPageLoaded(callback2);
expect(addEventListenerSpy).toHaveBeenCalledTimes(1);

// Both callbacks should be called
expect(callback1).not.toHaveBeenCalled();
expect(callback2).not.toHaveBeenCalled();
});
});
});
52 changes: 0 additions & 52 deletions spec/dummy/spec/system/integration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,58 +86,6 @@ def finished_all_ajax_requests?
include_examples "React Component", "div#my-hello-world-id"
end

shared_context "with pro features and immediate hydration" do
before do
allow(ReactOnRails::Utils).to receive_messages(
react_on_rails_pro?: true,
react_on_rails_pro_version: "",
rsc_support_enabled?: false
)

# Stub ReactOnRailsPro::Utils.pro_attribution_comment for all tests
# since react_on_rails_pro? is set to true by default
pro_module = Module.new
utils_module = Module.new do
def self.pro_attribution_comment
"<!-- Powered by React on Rails Pro (c) ShakaCode | Licensed -->"
end
end
stub_const("ReactOnRailsPro", pro_module)
stub_const("ReactOnRailsPro::Utils", utils_module)
end

around do |example|
ReactOnRails.configure { |config| config.immediate_hydration = true }
example.run
ReactOnRails.configure { |config| config.immediate_hydration = false }
end
end

describe "Turbolinks across pages", :js do
subject { page }

include_context "with pro features and immediate hydration"

it "changes name in message according to input" do
visit "/client_side_hello_world"
expect_change_text_in_dom_selector("#HelloWorld-react-component-0")
click_on "Hello World Component Server Rendered, with extra options"
expect_change_text_in_dom_selector("#my-hello-world-id")
end
end

describe "TurboStream send react component", :js do
subject { page }

include_context "with pro features and immediate hydration"

it "force load hello-world component immediately" do
visit "/turbo_frame_tag_hello_world"
click_on "send me hello-turbo-stream component"
expect(page).to have_text "Hello, Mrs. Client Side Rendering From Turbo Stream!"
end
end

describe "Pages/client_side_log_throw", :ignore_js_errors, :js do
subject { page }

Expand Down
Loading