Skip to content

fix(zone.js): patch node for nested fs functions#45552

Closed
lamweili wants to merge 2 commits intoangular:mainfrom
lamweili:patch-node-fs-nested
Closed

fix(zone.js): patch node for nested fs functions#45552
lamweili wants to merge 2 commits intoangular:mainfrom
lamweili:patch-node-fs-nested

Conversation

@lamweili
Copy link
Copy Markdown

@lamweili lamweili commented Apr 6, 2022 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Only direct fs functions are patched by zone.js/node.
After zone.js/node patches fs functions, nested fs functions become undefined.

  • fs.realpath.native is undefined
  • fs.realpathSync.native is undefined

Issue Number: #45546

What is the new behavior?

It patches the nested functions fs.realpath.native and fs.realpathSync.native first, storing them in a temporary space, before patching the usual fs functions (which will cause the nested functions to be undefined). Lastly, it populates back the patched nested functions from the temporary space back to fs.

  • fs.realpath.native is patched by zone.js/node and is no longer undefined
  • fs.realpathSync.native is patched by zone.js/node and is no longer undefined

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Fixes: #45546

@pullapprove pullapprove Bot requested a review from JiaLiPassion April 6, 2022 18:42
@lamweili lamweili mentioned this pull request Apr 6, 2022
Closed
@lamweili lamweili force-pushed the patch-node-fs-nested branch from 997b529 to b1905db Compare April 6, 2022 18:56
@lamweili lamweili mentioned this pull request Apr 6, 2022
Closed
@lamweili lamweili force-pushed the patch-node-fs-nested branch 2 times, most recently from 3413cf5 to 5da9b15 Compare April 6, 2022 19:17
@lamweili
fix(zone.js): patch node for nested fs functions
9a52aa1 
Closes angular#45546

It patches the nested functions `fs.realpath.native` and `fs.realpathSync.native` first, storing them in a temporary space, before patching the usual `fs` functions (which will cause the nested functions to be `undefined`). Lastly, it populates back the patched nested functions from the temporary space back to `fs`.
@lamweili lamweili force-pushed the patch-node-fs-nested branch from 5da9b15 to 9a52aa1 Compare April 6, 2022 19:36
@jessicajaniuk jessicajaniuk added the area: zones Issues related to zone.js label Apr 7, 2022
@ngbot ngbot Bot added this to the Backlog milestone Apr 7, 2022
@JiaLiPassion
Copy link
Copy Markdown
Contributor

JiaLiPassion commented Apr 8, 2022

@peteriman , thank you for the PR, could you add test case to make sure the method is patched correctly?

@lamweili
Copy link
Copy Markdown
Author

lamweili commented Apr 8, 2022 •
edited
Loading

Hi @JiaLiPassion, I would need some help on that and would need your advice.
Pardon my imprudence, I have contacted you privately.

@jessicajaniuk jessicajaniuk added the target: patch This PR is targeted for the next patch release label Apr 11, 2022
@dylhunn dylhunn marked this pull request as draft June 24, 2022 21:56
@dylhunn
Copy link
Copy Markdown
Contributor

dylhunn commented Jun 24, 2022

Converting this to draft since it doesn't seem to be ready for review, until the unit test issue is resolved

@JiaLiPassion
Copy link
Copy Markdown
Contributor

JiaLiPassion commented Jun 27, 2022

@lamweili , sorry I didn't have time to review this one, I will check it this week.

@lamweili
Copy link
Copy Markdown
Author

lamweili commented Sep 5, 2022

@JiaLiPassion, thanks. I have emailed you my questions on 8 April thereabouts previously. Have you gotten the time?

@JiaLiPassion
Copy link
Copy Markdown
Contributor

JiaLiPassion commented Sep 10, 2022

@lamweili , sorry for the late response, I updated the source and also added test cases, please check the test cases and make it complete and run

yarn bazel test //packages/zone.js/...

to make sure the test passed.

Then rebase the PR to the latest main branch content.

Thank you.

@JeanMeche
Copy link
Copy Markdown
Member

JeanMeche commented Jan 26, 2024

I'm closing old draft PRs coming from outside contributors. Feel free to ping if you'd like to keep it open.

@JeanMeche JeanMeche closed this Jan 26, 2024
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Feb 26, 2024

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Feb 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JiaLiPassion JiaLiPassion Awaiting requested review from JiaLiPassion

Assignees

No one assigned

Labels

area: zones Issues related to zone.js target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

zone.js/node breaks for fs.realpath.native and fs.realpathSync.native

5 participants

@lamweili @JiaLiPassion @dylhunn @JeanMeche @jessicajaniuk