Skip to content

Configurable file system via options.fs#370

Merged
alexander-akait merged 3 commits into
webpack:masterfrom
hinell:master
Feb 19, 2019
Merged

Configurable file system via options.fs#370
alexander-akait merged 3 commits into
webpack:masterfrom
hinell:master

Conversation

@hinell

@hinell hinell commented Feb 16, 2019

Copy link
Copy Markdown
Contributor

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

jsf-clabot commented Feb 16, 2019

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Feb 16, 2019

Copy link
Copy Markdown

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
Copy Markdown
Member

Please accept CLA, please describe use case

@hinell hinell mentioned this pull request Feb 18, 2019
3 tasks
Comment thread README.md
Comment thread README.md Outdated

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

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

Comment thread test/tests/file-system.js
@alexander-akait

alexander-akait commented Feb 18, 2019

Copy link
Copy Markdown
Member

Waiting CI green and merge 👍

Comment thread lib/fs.js
if (isMemoryFs) {
if (isConfiguredFs) {
const { fs } = context.options;
if (typeof fs.join !== 'function') {

Copy link
Copy Markdown
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
Copy Markdown
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.

@alexander-akait alexander-akait Feb 18, 2019

Copy link
Copy Markdown
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?

@hinell hinell Feb 18, 2019

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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.

3 participants