Skip to content

fix(profiling): Don't put require, __filename and __dirname on global object #14470

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

Jump to bottom
Merged
merged 33 commits into from
Jan 9, 2025
Merged

fix(profiling): Don't put require, __filename and __dirname on global object #14470

merged 33 commits into from
Jan 9, 2025

Conversation

@JonasBa
Copy link
Member

@JonasBa JonasBa commented Nov 25, 2024 •
edited by lforst
Loading

Fixes our CJS detection by no longer storing a reference to require on globalThis.

Ref #14525, #13662

@JonasBa JonasBa force-pushed the jb/profiling/local-require branch from fc0e866 to 531e99b Compare November 25, 2024 17:56
@JonasBa JonasBa force-pushed the jb/profiling/local-require branch from 531e99b to 7b07ab9 Compare November 25, 2024 20:10
@JonasBa JonasBa force-pushed the jb/profiling/local-require branch 2 times, most recently from f2ef50c to 5f6dba9 Compare November 30, 2024 14:20
@JonasBa JonasBa force-pushed the jb/profiling/local-require branch from 5f6dba9 to 98e4eec Compare November 30, 2024 14:51
@JonasBa JonasBa mentioned this pull request Dec 2, 2024
Closed
@JonasBa JonasBa force-pushed the jb/profiling/local-require branch 2 times, most recently from a45f224 to 32633bf Compare December 2, 2024 21:55
@JonasBa JonasBa force-pushed the jb/profiling/local-require branch from 32633bf to de1a11f Compare December 2, 2024 21:56
@JonasBa JonasBa marked this pull request as ready for review December 3, 2024 18:23
@lforst lforst self-assigned this Dec 12, 2024
@lforst lforst changed the title ref(profiling) test local require ref(profiling): Test local require Dec 12, 2024
@lforst lforst force-pushed the jb/profiling/local-require branch from 1eefeac to 5aa22a2 Compare December 12, 2024 12:12
@lforst lforst changed the title fix(profiling): Test local require fix(profiling): Don't put require, __filename and __dirname on global object Dec 12, 2024
@lforst lforst force-pushed the jb/profiling/local-require branch from 5f9438b to 4d46401 Compare December 12, 2024 13:47
@lforst lforst force-pushed the jb/profiling/local-require branch from 4d46401 to 31653f3 Compare December 12, 2024 13:49
@codecov
Copy link

codecov bot commented Dec 12, 2024 •
edited
Loading

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
615 1 614 294
View the full list of 1 ❄️ flaky tests 
client-app-routing-instrumentation.test.ts Creates a navigation transaction for app router routes

Flake rate in main: 10.71% (Passed 25 times, Failed 3 times)

Stack Traces | 30s run time 
client-app-routing-instrumentation.test.ts:19:5 Creates a navigation transaction for app router routes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@lforst
Copy link
Contributor

lforst commented Dec 13, 2024

I am gonna skip the test for now. I am not even sure what exactly we are testing here.

@JonasBa
Copy link
Member Author

JonasBa commented Dec 16, 2024 •
edited
Loading

@lforst I think it's fine to skip the test. The intention was to run a mjs script that uses profiling and check that globalThis.require was not available.

@Lms24 Lms24 mentioned this pull request Jan 3, 2025
Closed
3 tasks
mydea
mydea approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

I guess if tests pass, should be all good! Changes generally seem OK to me but I am not an expert on this matter ^^

@lforst lforst merged commit 3e73850 into develop Jan 9, 2025
173 checks passed
@lforst lforst deleted the jb/profiling/local-require branch January 9, 2025 09:54
@lforst lforst mentioned this pull request Jan 9, 2025
Merged
lforst pushed a commit that referenced this pull request Jan 9, 2025
fix(profiling/v8): Don't put require, __filename and __dirname …
785aab3 
…on global object (#14952)

Backports #14470 to v8.

Fixes #14525
Fixes #13662
@s1gr1d s1gr1d mentioned this pull request Jan 17, 2025
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mydea mydea mydea approved these changes

+1 more reviewer

@lforst lforst lforst approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lforst lforst

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JonasBa @lforst @mydea