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.

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

Remove the mkdirp dependency in favor of the built-in Node.js fs.mkdirSync #17935

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Apr 12, 2024

In Node.js 10.12.0 the recursive option was added to fs.mkdirSync. This option allows us to create a directory and all its parent directories if they do not exist, making it a drop-in replacement for the mkdirp dependency we use.

Given that PDF.js now requires Node.js 18+ we can be sure that this option is available, so we can safely remove mkdirp and reduce the number of project dependencies.

Thanks to @wojtekmaj, obviously credited as co-author of the commit, for providing the original implementation in #17696! For this patch I have extracted the mkdirp-only parts of that PR, with some minor changes, and tested all affected Gulp targets (using diff -r on the old/new build directories to recursively diff the contents of all directories and files) to make sure the output before/after this patch is identical.

…kdirSync`

In Node.js 10.12.0 the `recursive` option was added to `fs.mkdirSync`.
This option allows us to create a directory and all its parent
directories if they do not exist, making it a drop-in replacement for
the `mkdirp` dependency we use.

Given that PDF.js now requires Node.js 18+ we can be sure that this
option is available, so we can safely remove `mkdirp` and reduce the
number of project dependencies.

Co-authored-by: Wojciech Maj <kontakt@wojtekmaj.pl>
@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/c5504af50342874/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/c5504af50342874/output.txt

Total script time: 1.25 mins

Published

@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/f423fd1265d6552/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/1d596a03a562443/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/1d596a03a562443/output.txt

Total script time: 7.38 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/f423fd1265d6552/output.txt

Total script time: 20.18 mins

  • Integration Tests: FAILED

@timvandermeij
Copy link
Contributor Author

@wojtekmaj I have added you as a co-author of the commit, but GitHub can only partially verify the commit because we both enabled commit signing and it doesn't seem possible to double-sign a commit. Is that OK with you, or do you prefer that I remove the co-author property and credit you as a co-author in text in the commit message instead, to avoid having a commit that was authored but not signed by you?

@wojtekmaj
Copy link
Contributor

@timvandermeij No problem for me! It'd only be a problem if the repository enforced some kind of signed commits policy. To me, absolutely fine.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me, thank you.

@timvandermeij timvandermeij merged commit 14947ad into mozilla:master Apr 14, 2024
9 checks passed
@timvandermeij timvandermeij deleted the mkdirp branch April 14, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants