feat(options): Add an option for overriding the file system module in the JS API #5944
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5944 +/- ##
=======================================
Coverage 98.78% 98.78%
=======================================
Files 270 270
Lines 8733 8735 +2
Branches 1509 1510 +1
=======================================
+ Hits 8627 8629 +2
Misses 73 73
Partials 33 33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@lukastaegert I have fixed your suggestions |
There was a problem hiding this comment.
I took the liberty to have another look through the interface and did some more changes, please have a look.
- Looking at the flag and mode values, maybe it makes sense to keep them at
string|number. An ad-hoc implementation would most likely ignore those values anyway without causing harm. - I changed the type of file paths to be only
stringin a few more places. As it turns out, that also means we can get rid of the encoding option formkdtemp,readdirandrealpathas that was only about the encoding of file paths - The type of binary file data should not be
ArrayBuffer, that is an underlying utility type that is not well usable. It should beUint8Array, which is a typed array interface that exists in the browser but which is also implemented by the NodeJSBuffer. - It might be really useful to support
withFileTypes: trueinreaddir, and there was not much additional interface needed to support this if you limit to non-device directory entries. - Maybe it is fine to only support object options and not e.g. the short-hand buffer encoding for e.g.
readFile. I assume this was mostly a historical artifact in the NodeJs interfaces from a time when there only was one option.
Have another look if this looks fine from your side.
|
Amazing! Thank you! <3 |
# Conflicts: # package-lock.json # package.json
24edaef to
01f2edc
Compare
This should mostly work, the edge case that previously prompted it to be replaced is now handled gracefully. This will allow to make it much easier to use the browser build.
da8cde7 to
d36bc1a
Compare
There was a problem hiding this comment.
I had another look through it from the perspective how this might benefit the browser build. One thing I found is that now, browser/src/fs.ts is basically the default for the browser build, so I extended this file so that it covers all functions in the API and extended types to ensure this file is updated as needed.
Second, I noticed we no longer need the special resolveId logic for the browser. That has the effect that now, you can make the browser build run entirely via supplying an in-memory file system without any further need for plugins. This is really nice, and I used the chance to extend the documentation to explain this approach.
I also added docs for the plugin option, together with the recommendation to use it to make plugins NodeJS-independent.
|
Wow, amazing news, thank you! 🎉 |
976f3c9 to
0ef9681
Compare
|
This PR has been released as part of rollup@4.43.0. You can test it via |
Adds `this.fs` support which was added to rollup in rollup/rollup#5944. Note that while rollup/rollup#5944 adds `options.fs`, this PR does not add that feature.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
The problem is described in the issue #5880
The solution creates an internal RollupFsModule type to describe the file system option. It passes the option to the plugins as part of the context.