#172 - Support alternative fs implementations. #173

dazinator · 2019-03-19T15:41:03Z

Allow an alternative fs implementation to be used, overriding the default based on graceful-fs

jsf-clabot · 2019-03-19T15:41:05Z

All committers have signed the CLA.

codecov · 2019-03-19T15:42:53Z

Codecov Report

Merging #173 into master will increase coverage by 0.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   92.98%   92.99%   +0.01%     
==========================================
  Files          32       32              
  Lines        1140     1142       +2     
  Branches      262      263       +1     
==========================================
+ Hits         1060     1062       +2     
  Misses         80       80

Impacted Files	Coverage Δ
lib/NodeJsInputFileSystem.js	`36.84% <75%> (+7.43%)`	⬆️

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 5c1495a...109aab6. Read the comment docs.

webpack-bot · 2019-03-19T15:42:54Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

alexander-akait · 2019-03-19T15:50:52Z

Need add tests

dazinator · 2019-03-19T15:53:37Z

I'm typically a dotnet developer, node environment is failry new to me. I can run tests via mocha etc. In terms of adding a new test, any guidance as to where I should add it? Do I create a new file for this issue under the tests directoy? Or should I append a new test to an existing file somewhere? Thanks

alexander-akait · 2019-03-19T15:58:06Z

Create new file (example of test https://github.com/webpack/enhanced-resolve/blob/master/test/CachedInputFileSystem.js), test existing method with and without own fs

dazinator · 2019-03-19T16:25:23Z

I am seeing a lot of test failures on windows, i'll log under a seperate issue.

alexander-akait · 2019-03-19T16:29:29Z

@dazinator yep, we need fix their in separate issue, anyway feel free to send a PR with fixes too, a lot of developers work under linux so tests can be broken in some cases

dazinator · 2019-03-19T17:35:58Z

Have added some tests.

dazinator · 2019-03-23T19:31:54Z

I think this should meet the quality bar now but let me know if you need me to do anything further!

alexander-akait · 2019-03-25T10:03:24Z

test/NodeJsInputFileSystem.js

+	beforeEach(function() {});
+	afterEach(function() {
+		//fs.purge();
+	});


Need remove this

alexander-akait

All good, one note

webpack-bot · 2019-04-02T21:42:18Z

@dazinator Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@evilebottnawi Please review the new changes.

dazinator · 2019-04-02T21:42:45Z

@evilebottnawi Have made change requested.

sokra

I don't see why this change is needed. You can just write your own InputFileSystem class for your alternative filesystem. There is no major logic in NodeJsInputFileSystem.

dazinator · 2019-04-09T10:10:37Z

@sokra I did consider that. I guess it comes down to a choice.

If this class is changed to allow the 'fs' it works with to be injected, then it saves having to create essentially a duplicate.

I thought that creating a custom input file system could then be reserved for when:

you need to use something other than an 'fs' api to grab your files. Let's say you need to make a http call rec.
You actually have some custom logic to decide on the file

I guess my thinking is that I wanted to save a consumer from having to create a custom input file system when neither of the above were true..

Essentially widening the responsibility of the class from:

"This class is responsible for working with node's fs for file access'

to:

'This class is responsible for working with an fs for file access"

dazinator · 2019-04-27T13:55:52Z

Any further action required from me here or is this awaiting a decision

sokra · 2019-05-28T12:13:01Z

Essentially widening the responsibility of the class from:

"This class is responsible for working with node's fs for file access'

to:

'This class is responsible for working with an fs for file access"

The name is NodeJsFileSystem. It doesn't have a lot of logic. I don't think it makes sense to change this. sorry, but thanks for the PR anyway.

dazinator · 2019-05-28T12:44:44Z

Ok thanks for considering. I'll create a duplicate class.

dazinator · 2019-06-28T01:58:56Z

@sokra
I am adding feedback here because I recently stumbled across this PR again and it made me remember my frustration which I initially didn't voice. Do with this feedback what you will:
My only complaint is that having discussed this #172 and been told to submit a PR..
And then above being told to add tests..
Only then to be told the whole concept doesnt make sense to accept in the first place - sorry.
I know you must be busy but I wish you'd chimed in earlier. Time is an important resource for us all! That said I appreciate calls must be made, I just wish they were made a bit earlier in this particular case

webpack#172 - Support alternative fs implementations.

705727e

webpack-bot added PR: CI-ok PR: unreviewed labels Mar 19, 2019

dazinator mentioned this pull request Mar 19, 2019

Allow using an alternate fs for lookup of sources dependents/node-filing-cabinet#68

Closed

15 tasks

dazinator added 2 commits March 19, 2019 17:33

webpack#172 - Added tests.

4035a40

webpack#172 - Tidyup removed console log and try catch.

a431a92

dazinator mentioned this pull request Mar 19, 2019

[Question] - custom fs implementation #172

Closed

alexander-akait reviewed Mar 25, 2019

View reviewed changes

test/NodeJsInputFileSystem.js Outdated

beforeEach(function() {});

afterEach(function() {

//fs.purge();

});

Copy link

Member

alexander-akait Mar 25, 2019

Choose a reason for hiding this comment

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

Need remove this

alexander-akait requested changes Mar 25, 2019

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Mar 25, 2019

Tidy up test as per code review.

109aab6

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Apr 2, 2019

sokra requested changes Apr 8, 2019

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Apr 8, 2019

sokra closed this May 28, 2019

TomStrepsil mentioned this pull request Oct 24, 2024

Consider using memory filesystem compatible with webpack/enhanced-resolve/CachedInputFileSystem privatenumber/webpack-test-utils#6

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#172 - Support alternative fs implementations. #173

#172 - Support alternative fs implementations. #173

dazinator commented Mar 19, 2019

jsf-clabot commented Mar 19, 2019 •

edited

Loading

codecov bot commented Mar 19, 2019 •

edited

Loading

webpack-bot commented Mar 19, 2019

alexander-akait commented Mar 19, 2019

dazinator commented Mar 19, 2019

alexander-akait commented Mar 19, 2019

dazinator commented Mar 19, 2019

alexander-akait commented Mar 19, 2019

dazinator commented Mar 19, 2019

dazinator commented Mar 23, 2019

alexander-akait Mar 25, 2019

alexander-akait left a comment

webpack-bot commented Apr 2, 2019

dazinator commented Apr 2, 2019

sokra left a comment

dazinator commented Apr 9, 2019 •

edited

Loading

dazinator commented Apr 27, 2019

sokra commented May 28, 2019

dazinator commented May 28, 2019

dazinator commented Jun 28, 2019 •

edited

Loading

#172 - Support alternative fs implementations. #173

#172 - Support alternative fs implementations. #173

Conversation

dazinator commented Mar 19, 2019

jsf-clabot commented Mar 19, 2019 • edited Loading

codecov bot commented Mar 19, 2019 • edited Loading

Codecov Report

webpack-bot commented Mar 19, 2019

alexander-akait commented Mar 19, 2019

dazinator commented Mar 19, 2019

alexander-akait commented Mar 19, 2019

dazinator commented Mar 19, 2019

alexander-akait commented Mar 19, 2019

dazinator commented Mar 19, 2019

dazinator commented Mar 23, 2019

alexander-akait Mar 25, 2019

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

webpack-bot commented Apr 2, 2019

dazinator commented Apr 2, 2019

sokra left a comment

Choose a reason for hiding this comment

dazinator commented Apr 9, 2019 • edited Loading

dazinator commented Apr 27, 2019

sokra commented May 28, 2019

dazinator commented May 28, 2019

dazinator commented Jun 28, 2019 • edited Loading

jsf-clabot commented Mar 19, 2019 •

edited

Loading

codecov bot commented Mar 19, 2019 •

edited

Loading

dazinator commented Apr 9, 2019 •

edited

Loading

dazinator commented Jun 28, 2019 •

edited

Loading