refactor: use fs.promises #4314

dnalborczyk · 2021-12-22T01:09:43Z

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no
tests are refactored as well

Breaking Changes?

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

List any relevant issue numbers:

Description

since the code already has an async flow, I think it makes sense to use the fs promise functions replacing the synchronous (aka blocking) versions as well as the callback Promise wrapped functions. also added some additional typings.

note:
while import { promises as fs } from 'fs' is a bit on the uglier side of things, we can change it to import { readFile } from 'fs/promises' for v3 (if targeting node.js v14+)

codecov · 2021-12-22T01:15:06Z

Codecov Report

Merging #4314 (889ca7d) into master (bebc50d) will decrease coverage by 0.22%.
The diff coverage is 92.30%.

❗ Current head 889ca7d differs from pull request most recent head 2cea507. Consider uploading reports for the commit 2cea507 to get more accurate results

@@            Coverage Diff             @@
##           master    #4314      +/-   ##
==========================================
- Coverage   98.69%   98.46%   -0.23%     
==========================================
  Files         205      205              
  Lines        7331     7306      -25     
  Branches     2084     2082       -2     
==========================================
- Hits         7235     7194      -41     
- Misses         36       54      +18     
+ Partials       60       58       -2

Impacted Files	Coverage Δ
src/utils/resolveId.ts	`93.33% <66.66%> (-1.41%)`	⬇️
cli/run/getConfigPath.ts	`89.47% <100.00%> (+0.58%)`	⬆️
cli/run/index.ts	`100.00% <100.00%> (ø)`
cli/run/loadConfigFile.ts	`96.15% <100.00%> (ø)`
cli/run/watch-cli.ts	`91.89% <100.00%> (+2.18%)`	⬆️
src/utils/fs.ts	`100.00% <100.00%> (+18.18%)`	⬆️
src/utils/timers.ts	`56.71% <0.00%> (-25.43%)`	⬇️
src/Chunk.ts	`100.00% <0.00%> (ø)`
src/Module.ts	`100.00% <0.00%> (ø)`
... and 21 more

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 bebc50d...2cea507. Read the comment docs.

lukastaegert

Looks like a good improvement!

lukastaegert · 2021-12-22T06:10:08Z

src/utils/fs.ts

-	new Promise<string>((fulfil, reject) =>
-		fs.readFile(file, 'utf-8', (err, contents) => (err ? reject(err) : fulfil(contents)))
-	);
+export function readFile(file: string): Promise<string> {


I think we could inline this one as it only existed to do manual promise wrapping 😉

aaah, ok. I was under the impression that it had to be in that fs.ts file because of the browser build. that was just a bad guess tho without having looked into the details.

in that case, I'll remove it. 👍

lukastaegert · 2021-12-22T06:12:14Z

src/utils/fs.ts

 	const dir = dirname(path);
-	fs.mkdirSync(dir, { recursive: true });
+	await fs.mkdir(dir, { recursive: true });


While we are inlining things, considering this has recently been reduced to two lines and the only usage location is also a very short one, maybe inline it as well? But you can also keep it separate if you prefer.

dnalborczyk · 2021-12-22T20:21:06Z

just noticed the re-export of 'fs': export * from 'fs';. totally forgot resolveId. gonna fix that up too.

dnalborczyk · 2021-12-23T00:10:59Z

it seems the tests are having some timing issues now since everything is full async.

responsible for some of the failures is the order of module ids, which, in an async context, are non-deterministic. I suppose the fsSync methods (plus the callback Promise wrapper) somehow made those deterministic (or "deterministic enough").

kzc · 2022-01-19T16:30:33Z

The removal of blocking I/O in this PR could potentially improve CPU utilization in rollup for faster overall bundling speed. How much effort would be involved to rework the 9 failing tests due to the race conditions exposed by this PR?

Side note: the potential speed gains could be limited by the use of sync fs calls in the node-resolve and commonjs plugins:

$ grep -i '[^a]sync' node_modules/\@rollup/plugin-node-resolve/dist/cjs/index.js 
    pkgPath = fs.realpathSync(pkgPath);
      const pkgJsonString = fs__default["default"].readFileSync(pkgJsonPath, 'utf-8');

$ grep -i '[^a]sync' node_modules/\@rollup/plugin-commonjs/dist/index.js
    if (fs.existsSync(path.join(dirPath, 'package.json'))) {
        JSON.parse(fs.readFileSync(path.join(dirPath, 'package.json'), { encoding: 'utf8' })).main ||
    if (fs.statSync(path).isDirectory()) return true;
    for (const path$1 of glob__default["default"].sync(isNegated ? pattern.substr(1) : pattern)) {
  ${fs.readFileSync(normalizedPath, { encoding: 'utf8' })}
        const stats = fs.statSync(candidates[i]);
      const resolvedPath = normalizePathSlashes(resolve.sync(source, { basedir: path.dirname(id) }));

dnalborczyk · 2022-01-21T21:13:40Z

I haven't looked at this PR for a while as I was waiting for the mac ci run to stabilize first.

The removal of blocking I/O in this PR could potentially improve CPU utilization in rollup for faster overall bundling speed.

yeah, I was hoping for the same.

How much effort would be involved to rework the 9 failing tests due to the race conditions exposed by this PR?

good question. I vaguely remember it had to do with the sorting of imported module ids. let me rebase this to refresh my mind.

dnalborczyk · 2022-01-24T01:14:50Z

I'll get back to this one after: #4319

dnalborczyk · 2022-01-29T17:33:06Z

Side note: the potential speed gains could be limited by the use of sync fs calls in the node-resolve and commonjs plugins:

$ grep -i '[^a]sync' node_modules/\@rollup/plugin-node-resolve/dist/cjs/index.js 
    pkgPath = fs.realpathSync(pkgPath);
      const pkgJsonString = fs__default["default"].readFileSync(pkgJsonPath, 'utf-8');

$ grep -i '[^a]sync' node_modules/\@rollup/plugin-commonjs/dist/index.js
    if (fs.existsSync(path.join(dirPath, 'package.json'))) {
        JSON.parse(fs.readFileSync(path.join(dirPath, 'package.json'), { encoding: 'utf8' })).main ||
    if (fs.statSync(path).isDirectory()) return true;
    for (const path$1 of glob__default["default"].sync(isNegated ? pattern.substr(1) : pattern)) {
  ${fs.readFileSync(normalizedPath, { encoding: 'utf8' })}
        const stats = fs.statSync(candidates[i]);
      const resolvedPath = normalizePathSlashes(resolve.sync(source, { basedir: path.dirname(id) }));

@kzc we should probably look into the plugin code (eventually) as well.

dnalborczyk · 2022-01-29T17:35:48Z

I'll break this PR up into smaller PRs. that way, some non-test-breaking things can be pulled in separately.

dnalborczyk · 2022-02-04T21:54:57Z

superseded by:

#4371
#4372
#4376
#4386

lukastaegert approved these changes Dec 22, 2021

View reviewed changes

dnalborczyk force-pushed the fs-promises branch 3 times, most recently from 5c4978c to ca8887e Compare December 22, 2021 21:13

dnalborczyk added 6 commits January 21, 2022 16:32

refactor: use fs.promises

8923700

simplify + add return type

da004c5

use Array.prototype.includes

207ec96

convert findFile and addJsExtensionIfNecessary to async functions

3728610

re-export fs.promises.readFile

cb8d461

inline function

2cea507

dnalborczyk force-pushed the fs-promises branch 2 times, most recently from 5afb390 to 2cea507 Compare January 21, 2022 21:59

dnalborczyk force-pushed the fs-promises branch from de74a3a to 2cea507 Compare January 29, 2022 16:57

dnalborczyk marked this pull request as draft January 29, 2022 17:36

dnalborczyk closed this Feb 4, 2022

dnalborczyk deleted the fs-promises branch February 4, 2022 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use fs.promises #4314

refactor: use fs.promises #4314

dnalborczyk commented Dec 22, 2021 •

edited

codecov bot commented Dec 22, 2021 •

edited

lukastaegert left a comment

lukastaegert Dec 22, 2021

dnalborczyk Dec 22, 2021 •

edited

lukastaegert Dec 22, 2021

dnalborczyk commented Dec 22, 2021

dnalborczyk commented Dec 23, 2021 •

edited

kzc commented Jan 19, 2022

dnalborczyk commented Jan 21, 2022

dnalborczyk commented Jan 24, 2022

dnalborczyk commented Jan 29, 2022 •

edited

dnalborczyk commented Jan 29, 2022

dnalborczyk commented Feb 4, 2022

refactor: use fs.promises #4314

refactor: use fs.promises #4314

Conversation

dnalborczyk commented Dec 22, 2021 • edited

Description

codecov bot commented Dec 22, 2021 • edited

Codecov Report

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert Dec 22, 2021

Choose a reason for hiding this comment

dnalborczyk Dec 22, 2021 • edited

Choose a reason for hiding this comment

lukastaegert Dec 22, 2021

Choose a reason for hiding this comment

dnalborczyk commented Dec 22, 2021

dnalborczyk commented Dec 23, 2021 • edited

kzc commented Jan 19, 2022

dnalborczyk commented Jan 21, 2022

dnalborczyk commented Jan 24, 2022

dnalborczyk commented Jan 29, 2022 • edited

dnalborczyk commented Jan 29, 2022

dnalborczyk commented Feb 4, 2022

dnalborczyk commented Dec 22, 2021 •

edited

codecov bot commented Dec 22, 2021 •

edited

dnalborczyk Dec 22, 2021 •

edited

dnalborczyk commented Dec 23, 2021 •

edited

dnalborczyk commented Jan 29, 2022 •

edited