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

Configurable file system via options.fs #370

Merged
merged 3 commits into from
Feb 19, 2019

Conversation

hinell
Copy link
Contributor

@hinell hinell commented Feb 16, 2019

What kind of change does this PR introduce?

  • fs option which can be used to configure non-default file system
    (currently, it is forced to use memory-fs instead)

What are use cases?

  • The same as for middleware, but with opportunity to use other memory file systems (like memfs)

Did you add tests for your changes?

  • adds couple of tests

Summary

  • Solves longstanding problem of not having opportunity to use other non-memory-fs libraries and configure it manually either via webpackCompiler.outputFileSystem or middleware(..., ..., { fs: fileSystem }

Does this PR introduce a breaking change?

  • Negative

Changes:
* fs option which can be used to configure non-default file system
(it is forced to memory-fs in case it is not the same instance)
* adding two simple tests
@jsf-clabot
Copy link

jsf-clabot commented Feb 16, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Feb 16, 2019

Codecov Report

Merging #370 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
+ Coverage   96.85%   96.93%   +0.08%     
==========================================
  Files           7        7              
  Lines         254      261       +7     
==========================================
+ Hits          246      253       +7     
  Misses          8        8
Impacted Files Coverage Δ
lib/fs.js 95.91% <100%> (+0.68%) ⬆️

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 8e54af2...8743f97. Read the comment docs.

@alexander-akait
Copy link
Member

Please accept CLA, please describe use case

@hinell hinell mentioned this pull request Feb 18, 2019
3 tasks
README.md Outdated Show resolved Hide resolved
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Some little fixes for docs and we can merge

test/tests/file-system.js Show resolved Hide resolved
@alexander-akait
Copy link
Member

alexander-akait commented Feb 18, 2019

Waiting CI green and merge 👍

if (isMemoryFs) {
if (isConfiguredFs) {
const { fs } = context.options;
if (typeof fs.join !== 'function') {
Copy link
Member

Choose a reason for hiding this comment

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

Just questions, can you find place where we use fs.join? Looks like we should remove this/deprecated from code base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not used by the webpack-middleware but rather by wepback instead. Here it is as a precaution to avoid digging up webpack's erros. If you attempt to provide fs without join() webpack will complain about it when accessing webpack.outputFileSystem.

I can switch to Russian to explain more if you like.

Copy link
Member

Choose a reason for hiding this comment

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

I can switch to Russian to explain more if you like.

No need

It is not used by the webpack-middleware but rather by wepback instead. Here it is as a precaution to avoid digging up webpack's erros. If you attempt to provide fs without join() webpack will complain about it when accessing webpack.outputFileSystem.

I think we should open issue in webpack and remove all fs.join in webpack@5, join is not part of fs and looks very dirty and misleading

Can you open issue?

Copy link
Contributor Author

@hinell hinell Feb 18, 2019

Choose a reason for hiding this comment

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

Well I got no time for that, sorry. May be next time.

Copy link
Member

Choose a reason for hiding this comment

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

@hinell i mean just create issue in webpack repo and all, i am from mobile and it is hard for me right now

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