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

Jump to bottom

build: remove fs-extra devdep #42533

Merged
merged 18 commits into from
Jun 19, 2024
Merged

build: remove fs-extra devdep #42533

merged 18 commits into from
Jun 19, 2024

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Jun 17, 2024 •
edited

Description of Change

Node 20's fs module does everything we need now -- particularly with { recursive: true } -- so we can drop our fs-extra dev dependency.

I noticed this while tracking down an intermittent test failure in our downstream fork.

CC @codebytere

Checklist

Release Notes

Notes: none.

ckerr added 15 commits June 17, 2024 13:54
@ckerr
build: remove fs-extra dependency from script/gen-filenames.ts
6109a4a
@ckerr
build: remove fs-extra dependency from script/spec-runner.js
4b9ee88
@ckerr
build: remove fs-extra dependency from script/gn-asar.js
cce4181
@ckerr
build: remove fs-extra dependency from spec/api-autoupdater-darwin-sp…
1c4f77f 
…ec.ts
@ckerr
build: remove fs-extra dependency from spec/api-safe-storage-spec.ts
71fb08c
@ckerr
build: remove fs-extra dependency from spec/lib/codesign-helpers.ts
a208cc2
@ckerr
build: remove fs-extra dependency from spec/api-app-spec.ts
d5c1a92
@ckerr
build: remove fs-extra dependency from spec/esm-spec.ts
97cfdee
@ckerr
build: remove fs-extra dependency from spec/lib/fs-helpers.ts
315737d
@ckerr
build: remove fs-extra dependency from spec/lib/api-shell-spec.ts
5516b3f
@ckerr
build: remove fs-extra dependency from spec/api-context-bridge-spec.ts
2c09761
@ckerr
build: remove fs-extra dependency from spec/asar-integrity-spec.ts
81d3e89
@ckerr
build: remove fs-extra dependency from spec/node-spec.ts
03c7771
@ckerr
build: remove fs-extra devdiv
bbf1ce6
@ckerr
fixup! build: remove fs-extra dependency from spec/api-context-bridge…
f049d99 
…-spec.ts
@ckerr ckerr added semver/none target/31-x-y PR should also be added to the "31-x-y" branch. target/32-x-y PR should also be added to the "32-x-y" branch. labels Jun 17, 2024
jkleinsc
jkleinsc requested changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

The changes here fail to build. I think instead of refactoring bits here we should stick to the orginal code as much as possible (eg don't change async to sync).

script/spec-runner.js Outdated Show resolved Hide resolved
@electron electron deleted a comment from jkleinsc Jun 18, 2024
@ckerr
Copy link
Member Author

ckerr commented Jun 18, 2024

Oh crud. I meant to delete a draft reply and instead deleted one of @jkleinsc's code review comments instead. Sorry about that, John.

John's comment was that the new version of gn-asar.js does not catch an error thrown in fs.rm() and he's correct. In my defense, the same is true in the last line of the old version of the script too 😸

@electron electron deleted a comment from jkleinsc Jun 18, 2024
@ckerr
Copy link
Member Author

ckerr commented Jun 18, 2024

ckerr deleted a comment from jkleinsc now

Apparently I am just drunk with power and should put the keyboard away for the night

@ckerr ckerr requested a review from jkleinsc June 18, 2024 04:24
@codebytere codebytere self-requested a review June 18, 2024 10:09
ckerr added a commit that referenced this pull request Jun 28, 2024
@ckerr
build: remove fs-extra devdep (#42533)
f43980b 
* build: remove fs-extra dependency from script/gen-filenames.ts

* build: remove fs-extra dependency from script/spec-runner.js

* build: remove fs-extra dependency from script/gn-asar.js

* build: remove fs-extra dependency from spec/api-autoupdater-darwin-spec.ts

* build: remove fs-extra dependency from spec/api-safe-storage-spec.ts

* build: remove fs-extra dependency from spec/lib/codesign-helpers.ts

* build: remove fs-extra dependency from spec/api-app-spec.ts

* build: remove fs-extra dependency from spec/esm-spec.ts

* build: remove fs-extra dependency from spec/lib/fs-helpers.ts

* build: remove fs-extra dependency from spec/lib/api-shell-spec.ts

* build: remove fs-extra dependency from spec/api-context-bridge-spec.ts

* build: remove fs-extra dependency from spec/asar-integrity-spec.ts

* build: remove fs-extra dependency from spec/node-spec.ts

* build: remove fs-extra devdiv

* fixup! build: remove fs-extra dependency from spec/api-context-bridge-spec.ts

* fix: use force: true when removing directories

* chore: reduce diffs to main
@ckerr ckerr mentioned this pull request Jun 28, 2024
Closed
@trop

This comment was marked as outdated.

Sign in to view
@ckerr
Copy link
Member Author

ckerr commented Jun 28, 2024

/trop run backport-to 31-x-y

@trop
Copy link
Contributor

trop bot commented Jun 28, 2024

The backport process for this PR has been manually initiated - sending your PR to 31-x-y!

@trop trop bot mentioned this pull request Jun 28, 2024
Merged
@trop
Copy link
Contributor

trop bot commented Jun 28, 2024

I have automatically backported this PR to "31-x-y", please check out #42708

@trop trop bot added in-flight/31-x-y merged/31-x-y PR was merged to the "31-x-y" branch. and removed in-flight/31-x-y labels Jun 28, 2024
@ckerr
Copy link
Member Author

ckerr commented Jun 28, 2024

/trop run backport-to 30-x-y

@trop
Copy link
Contributor

trop bot commented Jun 28, 2024

The backport process for this PR has been manually initiated - sending your PR to 30-x-y!

@trop trop bot mentioned this pull request Jun 28, 2024
Merged
@trop
Copy link
Contributor

trop bot commented Jun 28, 2024

I have automatically backported this PR to "30-x-y", please check out #42711

@ckerr
Copy link
Member Author

ckerr commented Jun 28, 2024

/trop run backport-to 32-x-y

@trop
Copy link
Contributor

trop bot commented Jun 28, 2024

The backport process for this PR has been manually initiated - sending your PR to 32-x-y!

@trop trop bot mentioned this pull request Jun 28, 2024
Merged
@trop
Copy link
Contributor

trop bot commented Jun 28, 2024

I have automatically backported this PR to "32-x-y", please check out #42712

@trop trop bot added in-flight/32-x-y merged/30-x-y PR was merged to the "30-x-y" branch. merged/32-x-y PR was merged to the "32-x-y" branch. and removed in-flight/32-x-y in-flight/30-x-y labels Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
merged/30-x-y PR was merged to the "30-x-y" branch. merged/31-x-y PR was merged to the "31-x-y" branch. merged/32-x-y PR was merged to the "32-x-y" branch. semver/none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ckerr @jkleinsc @codebytere