chore: replace mkdirp with mkdir fs #46388
Conversation
|
Hi @anirudhdream11! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
facebook-github-bot
added
CLA Signed
| if (folder === outputDirectory) { | ||
| expect(options).toEqual({ recursive: true }); | ||
| } |
There was a problem hiding this comment.
This test does not verify that the first call was for value in outputDirectory.
It attempts to verify that if any call included value from outputDirectory, it contained options set to { recursive: true } which isn't the same.
There was a problem hiding this comment.
I think the testing should be similar to how it's done in scripts/__tests__/scm-utils-test.js
Create mock function using jest.fn(), and validate it's Nth calls using https://jestjs.io/docs/expect#tohavebeennthcalledwithnthcall-arg1-arg2-
There was a problem hiding this comment.
Thanks for the feedback @trivikr
I've updated the test to ensure it explicitly verifies that mkdirSync is called exactly once with outputDirectory and the correct options { recursive: true }. Since the function logic only requires one directory creation, I removed unnecessary checks for multiple invocations.
There was a problem hiding this comment.
Verified update in commit b8c406c (#46388).
I don't have permission to resolve this thread though.
packages/react-native/scripts/codegen/__tests__/generate-specs-cli-executor-test.js
Show resolved
Hide resolved
packages/react-native/scripts/codegen/__tests__/generate-specs-cli-executor-test.js
Outdated
Show resolved
Hide resolved
…-cli-executor-test.js Co-authored-by: Alex Hunt <hello@alexhunt.io>
…into chore/replace-mkdirp-fs
…ream11/react-native into chore/replace-mkdirp-fs
|
Hi @cortinico I’ve made some changes, including:
Could you please take a look when you have some time? I'd appreciate any additional feedback or approval if everything looks good. |
|
@anirudhdream11 Changes look good but we can't move forward until we have passing CI. I think this is due to Prettier! Please run |
…into chore/replace-mkdirp-fs
|
@huntie has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was successfully merged by @anirudhdream11 in 7cdb87e When will my fix make it into a release? | How to file a pick request? |
Summary:
This pull request replaces the use of mkdirp with Node.js's built-in fs.mkdirSync({ recursive: true }) function, which is available in Node.js version 10.12.0 and above. This change reduces the number of external dependencies and simplifies the codebase by using the native capabilities of Node.js.
The motivation behind this change is to remove the unnecessary mkdirp dependency, as Node.js natively supports recursive directory creation since version 10.12.0. This streamlines the code and reduces the reliance on external libraries.
Changelog:
[INTERNAL] [REMOVED] - Replaced mkdirp with fs.mkdirSync({ recursive: true }) in build scripts and codegen. Requires Node.js 10.12.0 and above.
Test Plan:
I ran the build and codegen scripts locally with Node.js version 10.12.0 and above after replacing mkdirp, ensuring the scripts work as expected. No issues were encountered, and all processes, including directory creation and file handling, function correctly.