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

fix(node): use eval for require to avoid bundler issue #3239

Merged
merged 4 commits into from
Nov 18, 2024
Merged

fix(node): use eval for require to avoid bundler issue #3239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0267d1d
fix(node): use eval for require to avoid bundler issue
ScriptedAlchemy Nov 17, 2024
451fcad
Merge branch 'main' into use-eval-require
ScriptedAlchemy Nov 17, 2024
eb61e96
Merge branch 'main' into use-eval-require
ScriptedAlchemy Nov 17, 2024
d6e8545
chore: set local webpack in ci
ScriptedAlchemy Nov 17, 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
5 changes: 5 additions & 0 deletions .changeset/eighty-schools-smell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@module-federation/node': patch
---

use eval for require to avoid bundler warnings
3 changes: 3 additions & 0 deletions .github/workflows/e2e-next-dev.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ jobs:
- name: Set SKIP_DEVTOOLS_POSTINSTALL environment variable
run: echo "SKIP_DEVTOOLS_POSTINSTALL=true" >> $GITHUB_ENV

- name: Set local webpack
run: echo "NEXT_PRIVATE_LOCAL_WEBPACK=true" >> $GITHUB_ENV

- name: Install Dependencies
run: pnpm install

Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/utils/hot-reload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const getRequire = (): NodeRequire => {
//@ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

The @ts-ignore comment should be replaced with a more specific @ts-expect-error to better document the expected type error. This helps catch issues if the type error is resolved in the future.

return typeof __non_webpack_require__ !== 'undefined'
? __non_webpack_require__
: require;
: eval('require');
Comment on lines 12 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

The current implementation using eval('require') poses security risks and may be blocked by Content Security Policy (CSP). A safer alternative would be to use Function constructor or check for the global require object:

Suggested change
//@ts-ignore
return typeof __non_webpack_require__ !== 'undefined'
? __non_webpack_require__
: require;
: eval('require');
//@ts-ignore
return typeof __non_webpack_require__ !== 'undefined'
? __non_webpack_require__
: Function('return require')();

Additionally, consider adding a comment explaining why this workaround is necessary for webpack bundling to help future maintainers understand the purpose of this code.

Copy link

Choose a reason for hiding this comment

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

Yep, we have this CSP problem :(
@ScriptedAlchemy

};

function callsites(): any[] {
Expand Down
Loading