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

[WIP] Fix error of overwriting window.ReactOnRails #1684

Closed
wants to merge 1 commit into from
Closed

[WIP] Fix error of overwriting window.ReactOnRails #1684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions node_package/src/ReactOnRails.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import ReactOnRails from './ReactOnRails';
import { setReactOnRails } from './context';
import streamServerRenderedReactComponent from './streamServerRenderedReactComponent';

ReactOnRails.streamServerRenderedReactComponent = streamServerRenderedReactComponent;

export * from './ReactOnRails';
export { default } from './ReactOnRails';
setReactOnRails(ReactOnRails);

export default ReactOnRails;
export * from './types';
27 changes: 3 additions & 24 deletions node_package/src/ReactOnRails.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import serverRenderReactComponent from './serverRenderReactComponent';
import buildConsoleReplay from './buildConsoleReplay';
import createReactOutput from './createReactOutput';
import Authenticity from './Authenticity';
import context from './context';
import type {
RegisteredComponent,
RenderParams,
Expand All @@ -19,31 +18,16 @@ import type {
AuthenticityHeaders,
Store,
StoreGenerator,
ReactOnRails as ReactOnRailsType,
} from './types';
import reactHydrateOrRender from './reactHydrateOrRender';

const ctx = context();

if (ctx === undefined) {
throw new Error("The context (usually Window or NodeJS's Global) is undefined.");
}

if (ctx.ReactOnRails !== undefined) {
throw new Error(`
The ReactOnRails value exists in the ${ctx} scope, it may not be safe to overwrite it.

This could be caused by setting Webpack's optimization.runtimeChunk to "true" or "multiple," rather than "single." Check your Webpack configuration.

Read more at https://github.com/shakacode/react_on_rails/issues/1558.
`);
}

const DEFAULT_OPTIONS = {
traceTurbolinks: false,
turbo: false,
};

ctx.ReactOnRails = {
const ReactOnRails: ReactOnRailsType = {
options: {},
/**
* Main entry point to using the react-on-rails npm package. This is how Rails will be able to
Expand Down Expand Up @@ -299,9 +283,4 @@ ctx.ReactOnRails = {
},
};

ctx.ReactOnRails.resetOptions();

ClientStartup.clientStartup(ctx);

export * from "./types";
export default ctx.ReactOnRails;
export default ReactOnRails;
7 changes: 7 additions & 0 deletions node_package/src/ReactOnRails.web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import ReactOnRails from './ReactOnRails';
import { setReactOnRails } from './context';

setReactOnRails(ReactOnRails);

export default ReactOnRails;
export * from './types';
29 changes: 28 additions & 1 deletion node_package/src/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,36 @@
import { ReactOnRails as ReactOnRailsType } from './types';
import * as ClientStartup from './clientStartup';

/**
* Get the context, be it window or global
* @returns {boolean|Window|*|context}
*/
export default function context(this: void): Window | NodeJS.Global | void {
export function context(this: void): Window | NodeJS.Global | void {
return ((typeof window !== 'undefined') && window) ||
((typeof global !== 'undefined') && global) ||
this;
}
Comment on lines +8 to 12
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

Fix return type to address static analysis warning.

The void type in the union is flagged as confusing. Consider using undefined instead.

-export function context(this: void): Window | NodeJS.Global | void {
+export function context(this: void): Window | NodeJS.Global | undefined {
   return ((typeof window !== 'undefined') && window) ||
     ((typeof global !== 'undefined') && global) ||
-    this;
+    undefined;
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function context(this: void): Window | NodeJS.Global | void {
return ((typeof window !== 'undefined') && window) ||
((typeof global !== 'undefined') && global) ||
this;
}
export function context(this: void): Window | NodeJS.Global | undefined {
return ((typeof window !== 'undefined') && window) ||
((typeof global !== 'undefined') && global) ||
undefined;
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 8-8: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


export const setReactOnRails = (value: ReactOnRailsType) => {
const ctx = context();

if (ctx === undefined) {
throw new Error("The context (usually Window or NodeJS's Global) is undefined.");
}

if (ctx.ReactOnRails !== undefined) {
throw new Error(`
The ReactOnRails value exists in the ${ctx} scope, it may not be safe to overwrite it.

This could be caused by setting Webpack's optimization.runtimeChunk to "true" or "multiple," rather than "single." Check your Webpack configuration.

Read more at https://github.com/shakacode/react_on_rails/issues/1558.
`);
}

ctx.ReactOnRails = value;

ctx.ReactOnRails.resetOptions();

ClientStartup.clientStartup(ctx);
};
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"exports": {
".": {
"node": "./node_package/lib/ReactOnRails.node.js",
"default": "./node_package/lib/ReactOnRails.js"
"default": "./node_package/lib/ReactOnRails.web.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Tests needed for dual environment exports configuration

The current test suite doesn't cover the new conditional exports feature. Please add tests to verify:

  • Module loading in Node.js environment using ReactOnRails.node.js
  • Module loading in web environment using ReactOnRails.web.js
  • Correct resolution of exports based on environment
🔗 Analysis chain

Consider adding tests for the new export configuration.

Since this change affects how the package is imported and used, ensure there are tests covering both Node.js and web environments.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for existing test files related to exports
fd -e test.ts -e test.js -e spec.ts -e spec.js node_package/tests/

Length of output: 450

Script:

#!/bin/bash
# Find all test files in node_package directory
fd -e test.js -e test.ts -e spec.js -e spec.ts . node_package

# Search for export-related test content
rg -l "ReactOnRails.web" --type-add 'test:*.{test,spec}.{js,ts}' -t test

# Check package.json exports configuration
cat package.json

Length of output: 3548

Script:

#!/bin/bash
# Check content of main test file
cat node_package/tests/ReactOnRails.test.js

# Search for environment-specific testing patterns
rg -A 5 "describe|it\(" node_package/tests/ReactOnRails.test.js

# Look for import/require statements in tests
rg "require\(.*ReactOnRails\)|import.*ReactOnRails" node_package/tests/

Length of output: 6176

}
},
"directories": {
Expand Down
Loading