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

refactor: use fs, fs-extra, remove sander #4319

Merged
merged 11 commits into from Jan 29, 2022

Conversation

dnalborczyk
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor (tests)
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

@dnalborczyk dnalborczyk changed the title refactor: use fs-extra.readFileSync refactor: use fs-extra Dec 26, 2021
@codecov
Copy link

codecov bot commented Dec 26, 2021

Codecov Report

Merging #4319 (c637a86) into master (7194b88) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4319   +/-   ##
=======================================
  Coverage   98.70%   98.70%           
=======================================
  Files         205      205           
  Lines        7320     7320           
  Branches     2083     2083           
=======================================
  Hits         7225     7225           
  Misses         35       35           
  Partials       60       60           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7194b88...c637a86. Read the comment docs.

@dnalborczyk dnalborczyk changed the title refactor: use fs-extra refactor: use fs, fs-extra, remove sander Dec 26, 2021
@kzc
Copy link
Contributor

kzc commented Dec 26, 2021

#4318 replaces fs.writeFileSync and sander.writeFileSync with atomicWriteFileSync for reasons described in the link.

@kzc
Copy link
Contributor

kzc commented Dec 26, 2021

In an experiment, replacing await sander.copydir() with an equivalent execSync call copying the directory via shell commands did not improve the reliability of certain node12/macos watch tests. Curiously, I haven't seen these watch test failures in node16/macos. Not sure if this is due to node16 being a faster JIT VM, or whether a NodeJS bug on macos was fixed after node12, or if there is some other issue with the tests themselves, chokidar, or rollup.

@kzc
Copy link
Contributor

kzc commented Dec 26, 2021

Curiously, I haven't seen these watch test failures in node16/macos.

I spoke too soon - here's a rare node16/macos failure in the commits of this PR:

https://github.com/rollup/rollup/runs/4634674788?check_suite_focus=true

@kzc kzc mentioned this pull request Dec 26, 2021
9 tasks
@dnalborczyk dnalborczyk force-pushed the fs-extra branch 7 times, most recently from d8c6dff to 465b77b Compare December 26, 2021 22:03
@dnalborczyk
Copy link
Contributor Author

In an experiment, replacing await sander.copydir() with an equivalent execSync call copying the directory via shell commands did not improve the reliability of certain node12/macos watch tests.

yeah, I didn't think the watch failures would go away with this. I agree with you that the timing and execution of the tests is likely at fault. I wanted to look after the last commits at the failures and/or wait for the other PR first.

I went a little further and made this PR also fully async.

@lukastaegert
Copy link
Member

I like the idea of getting rid of sander, but maybe you are trying too much here by "fixing everything". Why not limit this one to replacing sander for now but keep especially the structure of the watch tests intact, and then improve them separately?

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Jan 22, 2022

I like the idea of getting rid of sander, but maybe you are trying too much here by "fixing everything". Why not limit this one to replacing sander for now but keep especially the structure of the watch tests intact, and then improve them separately?

good point. I haven't looked at this PR for a while because of the mac os ci failures.
let me rebase and revert some stuff to make it stable.

@dnalborczyk dnalborczyk marked this pull request as ready for review January 23, 2022 03:59
@dnalborczyk dnalborczyk mentioned this pull request Jan 24, 2022
10 tasks
@dnalborczyk
Copy link
Contributor Author

@lukastaegert this should be good to go

@dnalborczyk dnalborczyk force-pushed the fs-extra branch 2 times, most recently from 5ed64c7 to cd505fc Compare January 29, 2022 01:21
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Great work 👍

@lukastaegert lukastaegert merged commit 042933a into rollup:master Jan 29, 2022
@dnalborczyk dnalborczyk deleted the fs-extra branch February 3, 2022 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants