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: fs.promises, move mkdir to writeoutputfile, Part 3 #4376

Merged
merged 5 commits into from Feb 2, 2022

Conversation

dnalborczyk
Copy link
Contributor

@dnalborczyk dnalborczyk commented Jan 30, 2022

This PR contains:

  • bugfix
  • feature
  • refactor
  • 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

moves mkdir to writeOutputFile so it'll be called only once per file (not again for the source map file).

@lukastaegert do you think it would make sense to drop those in a Set, and only create it once for sure? might improve perf.

I'm guessing it can contain duplicates for:

  • chunks?
  • multiple rollup entry points? (might be the same as for chunks)
  • multiple configs? (not sure if rollup spins up a new fresh instance or works with the existing?)
  • possibly more?

Note: I had the changes and breaking tests in this PR originally, but I'll move 'em to Part 4.

@codecov
Copy link

codecov bot commented Jan 30, 2022

Codecov Report

Merging #4376 (0e1b321) into master (f420a01) will decrease coverage by 0.00%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4376      +/-   ##
==========================================
- Coverage   98.72%   98.72%   -0.01%     
==========================================
  Files         205      204       -1     
  Lines        7320     7315       -5     
  Branches     2083     2082       -1     
==========================================
- Hits         7227     7222       -5     
  Misses         34       34              
  Partials       59       59              
Impacted Files Coverage Δ
src/utils/resolveId.ts 93.33% <66.66%> (-1.41%) ⬇️
src/rollup/rollup.ts 100.00% <100.00%> (ø)

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 f420a01...0e1b321. Read the comment docs.

@dnalborczyk dnalborczyk changed the title refactor: use fs.promises in module loader, Part 3 refactor: use fs.promises in resolve id, Part 3 Jan 30, 2022
@dnalborczyk dnalborczyk force-pushed the fs-promises-part-3 branch 3 times, most recently from 1bab075 to 14df5a1 Compare January 30, 2022 20:23
@dnalborczyk dnalborczyk marked this pull request as ready for review January 30, 2022 20:33
@dnalborczyk dnalborczyk changed the title refactor: use fs.promises in resolve id, Part 3 refactor: fs.promises, move mkdir to writeoutputfile, Part 3 Jan 30, 2022
@lukastaegert
Copy link
Member

do you think it would make sense to drop those in a Set, and only create it once for sure? might improve perf.

Yes, but I think there are some things to consider:

  • Such Sets should be reset per-build (e.g. in watch mode), because directories could be removed again (e.g. there might be a plugin clearing the output folder before every build, therefore all directories should be recreated per build. Or directories are removed manually)
  • It would make sense to cache a lot more stuff per build. One thing that REALLY costs performance in my observation is using path.resolve. It seems to me that this method might actually be doing synchronous blocking file system IO to e.g. resolve symlinks. That is why using node-resolve is actually faster than vanilla JS: Because it is caching directory reads and doing all directory resolution manually via https://www.npmjs.com/package/resolve, which actually comes from browserify. I would not use that package in rollup core, though, instead I wonder if Rollup should not provide its own abstraction over the file system that uses a cache that is reset for each build. If that is made available to plugins, we could make many plugins browser/Deno compatible/independent of the runtime while making everything faster. And no longer have node-resolve rely on resolve.

Now the second point is independent of the first, but it makes we wonder in how far we want to think about a more general solution.

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.

Very nice!

@lukastaegert lukastaegert enabled auto-merge (squash) February 2, 2022 06:02
@lukastaegert lukastaegert merged commit d3ed80c into rollup:master Feb 2, 2022
@dnalborczyk dnalborczyk deleted the fs-promises-part-3 branch February 4, 2022 02:44
@dnalborczyk
Copy link
Contributor Author

Yes, but I think there are some things to consider:

@lukastaegert didn't ignore this, I'll get back to you on that.

@dnalborczyk dnalborczyk mentioned this pull request Feb 4, 2022
10 tasks
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

2 participants