Feature: add option "serveIndex" to enable/disable serveIndex middleware

[EslamHiko]

• This is a bugfix
• This is a feature
• This is a code refactor
• This is a test update
• This is a docs update
• This is a metadata update

For Bugs and Features; did you add new tests?

I used the examples in the example folder

Motivation / Use-Case

add this feature #1745

Breaking Changes

it disables/enables serveIndex

Additional Info

[codecov]

Codecov Report

Merging #1752 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1752      +/-   ##
=========================================
+ Coverage   87.54%   87.6%   +0.06%     
=========================================
  Files           9       9              
  Lines         586     589       +3     
  Branches      173     175       +2     
=========================================
+ Hits          513     516       +3     
  Misses         61      61              
  Partials       12      12

Impacted Files	Coverage Δ
lib/Server.js	83.29% <100%> (+0.12%)	⬆️

---

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 abf8691...f29502f. Read the comment docs.

[EslamHiko]

if I got it right

I tried to create a test
the logic of the test is very simple
1 - create a folder with a simple file in it
2 - set the option to true and check if it's listing the files in the directory (status:200)
3 - set the option to false and check if it's not listing the files in the directory (status:404)
4 - remove the folder
I managed to create the folder and remove it but whenever I test /folder route it returns status:301 Moved Permanently if it exists or not.
I need some help here to implement the test.

[alexander-akait]

Let's add tests

[EslamHiko]

@evilebottnawi here is a minimum reproducible test repo : https://github.com/EslamHiko/webpack-dev-server/tree/serve-index-option-test
True scenarios :
default settings testing :
1 - go to examples/api/simple
2- node server
3- go to http://localhost:8080/folder/ it'll directory list simple.txt

serveIndex:true testing :
1- edit webpack.config.js in the examples/api/simple folder
2- change the settings like here:

module.exports = setup({
  context: __dirname,
  entry: ['./app.js', '../../../client/index.js?http://localhost:8080/'],
  output: {
    filename: 'bundle.js',
  },
  devServer:{
    serveIndex:true
  }
});

3- node server
4- go to http://localhost:8080/folder/ it'll directory list simple.txt

False scenarios :
1- edit webpack.config.js in the examples/api/simple folder
2- change the settings like here:

module.exports = setup({
  context: __dirname,
  entry: ['./app.js', '../../../client/index.js?http://localhost:8080/'],
  output: {
    filename: 'bundle.js',
  },
  devServer:{
    serveIndex:false
  }
});

3- node server
4- go to http://localhost:8080/folder/ it'll show you the message Cannot GET /folder/

I wanted to write a jest test but i got status:301 Moved Permanently error because tests run in the root folder.
if there is any ideas to create a jest test for it i'll be so happy to know about them.

[alexander-akait]

Don't use typeof, we prefer using !

[EslamHiko]

      (options.serveIndex || options.serveIndex === undefined)

@evilebottnawi done

[alexander-akait]

Just simplify:

const shouldHandleServeIndex = contentBase || options.serveIndex

serveIndex by default should be true

[EslamHiko]

@evilebottnawi i've simplified the condition & i added a comment to elaborate why i'm comparing with undefined

          if (contentBase !== false) {
    // checking if it's set to true or not set (Default : undefined => true)
    options.serveIndex = options.serveIndex || options.serveIndex === undefined;

    const shouldHandleServeIndex = contentBase && options.serveIndex;

    if (shouldHandleServeIndex) {
      defaultFeatures.push('contentBaseIndex');
    }

[alexander-akait]

Better avoid modify options because code below also can use options and modification original options is not good idea better keep this in separate variable, it is good practice.

[EslamHiko]

@evilebottnawi thank you for the advice, I set the option value to a separate variable this.serveIndex

[alexander-akait]

Looks good, but we need add test for this

[EslamHiko]

@evilebottnawi here is a minimum reproducible test repo with the new code: https://github.com/EslamHiko/webpack-dev-server/tree/serve-index-option-test2

testing is the same as this comment :

@evilebottnawi here is a minimum reproducible test repo : https://github.com/EslamHiko/webpack-dev-server/tree/serve-index-option-test
True scenarios :
default settings testing :
1 - go to examples/api/simple
2- node server
3- go to http://localhost:8080/folder/ it'll directory list simple.txt

serveIndex:true testing :
1- edit webpack.config.js in the examples/api/simple folder
2- change the settings like here:

module.exports = setup({
  context: __dirname,
  entry: ['./app.js', '../../../client/index.js?http://localhost:8080/'],
  output: {
    filename: 'bundle.js',
  },
  devServer:{
    serveIndex:true
  }
});

3- node server
4- go to http://localhost:8080/folder/ it'll directory list simple.txt

False scenarios :
1- edit webpack.config.js in the examples/api/simple folder
2- change the settings like here:

module.exports = setup({
  context: __dirname,
  entry: ['./app.js', '../../../client/index.js?http://localhost:8080/'],
  output: {
    filename: 'bundle.js',
  },
  devServer:{
    serveIndex:false
  }
});

3- node server
4- go to http://localhost:8080/folder/ it'll show you the message Cannot GET /folder/

I wanted to write a jest test but i got status:301 Moved Permanently error because tests run in the root folder.
if there is any ideas to create a jest test for it i'll be so happy to know about them.

if there is any way to use jest to browse folders it'll be great to create a jest test for it.

[alexander-akait]

We have test for contentBase, so test should be same

[EslamHiko]

@evilebottnawi I added valid tests testing True,False & default settings, you wouldn't believe what caused the 301 Error i was doing req.get('/assets') instead of req.get('/assets/') that what prevented me from submitting my first test 😅 in the end it's working now without any issues.

[alexander-akait]

/cc @hiroppy

[hiroppy]

Please add the following test here because it is confirmed in serveIndex:false.

    it('should show Heyo. because bar has index.html inside it (200)', (done) => {
      req.get('/bar/').expect(200, /Heyo/, done);
    });

[hiroppy]

ditto

[EslamHiko]

@hiroppy Done

[alexander-akait]

/cc @hiroppy

[hiroppy]

LGTM, Thanks!

[EslamHiko]

@hiroppy You're Welcome 😄