Server index improvements

[DeMoorJasper]

• Closes 🐛 publicURL with development server #782
• closes 🐛 --public-url is setted on build and watch #714
• Closes HTML bundle serving bug. #974
• Changes default publicURL to / which is probably a better default
• Changes index bahaviour to be more inline with a standard webserver only showing index if the request url is publicURL or below publicURL (e.g. publicURL = /this/is/a/directory than it will give index on any path up to /this/is/a/directory/ once you go above that for example /this/is/a/directory/index.js it'll return an asset or 404 depending on it's existance)

[codecov-io]

Codecov Report

Merging #838 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #838     +/-   ##
=========================================
+ Coverage   91.46%   91.67%   +0.2%     
=========================================
  Files          68       68             
  Lines        3573     3687    +114     
=========================================
+ Hits         3268     3380    +112     
- Misses        305      307      +2

Impacted Files	Coverage Δ
src/Bundler.js	95.17% <ø> (+2.36%)	⬆️
src/Server.js	91.8% <100%> (+0.13%)	⬆️
src/Logger.js	81.9% <0%> (-11.12%)	⬇️
src/visitors/matches-pattern.js	92% <0%> (-8%)	⬇️
src/HMRServer.js	86.95% <0%> (-3.52%)	⬇️
src/packagers/index.js	88.88% <0%> (-3.42%)	⬇️
src/assets/TypeScriptAsset.js	97.5% <0%> (-2.5%)	⬇️
src/utils/installPackage.js	91.22% <0%> (-2.43%)	⬇️
src/packagers/CSSPackager.js	85.71% <0%> (-2.17%)	⬇️
src/assets/SASSAsset.js	89.58% <0%> (-1.91%)	⬇️
... and 42 more

---

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 246edd4...c857fa5. Read the comment docs.

[mightyiam]

Why set it to options.outDir? What does that provide? Why are these two related? I notice that this is the part that is not changed. I think we should change it, as I described in #932. To simply / by default.

[DeMoorJasper]

I didn’t want to change it too much, just fix a bug and prevent future issues regarding the default. Although i do agree with you, would be nice to have Devon’s opinion on these changes

[mightyiam]

function respond() {
      if (bundler.errored) {
        return send500();
      } else if (!req.url.startsWith(bundler.options.publicURL)) {
        return sendIndex();
      } else if (req.url === bundler.options.publicURL) {
        return sendIndex();
      } else {
        // Otherwise, serve the file from the dist folder
        req.url = req.url.slice(bundler.options.publicURL.length);
        return serve(req, res, send404);
      }
    }

[DeMoorJasper]

I don’t think this is a better solution as it just adds an extra unnecessary if

[mightyiam]

More readable for me.

[davidnagli]

I do agree with @mightyiam that the multiline || is pretty hard to read / reason about, but I also agree with @DeMoorJasper that the split ifs looks redundant and unnecessary since both ifs do exactly the same thing.

I would write it like this, which solves both of those problems, and get's rid of some of the confusing negation, making it much easier to understand and reason about:

function respond() {
  // If an error occurred in the bundler, send a 500 error
  if (bundler.errored) return send500();

  // If the URL start with the public path, serve the file from the dist folder
  if (req.url.startsWith(bundler.options.publicURL)) {
    // Check that its not the publicURL itself
    if (req.url !== bundler.options.publicURL) {
      req.url = req.url.slice(bundler.options.publicURL.length);
      return serve(req, res, send404);
    }
  }

  // Otherwise send the main HTML bundle
  return sendIndex();
}

Explanation

This converts the "or" into a negated "and" ([using this fancy Boolean algebra rule](https://en.wikipedia.org/wiki/De_Morgan%27s_laws)), then flips the resulting if-else to clean up the negation, and then splits the resulting `&&` into separate nested `if`s (thank you Webstorm for doing all that in the click of a button 😛). Oh, and I also got rid of all the `else`s, since they're redundant with the `return`s

 

IMO, my version is slightly cleaner and easier to read / reason about, but tbh it doesn't really matter so I'll leave it up to you.

[DeMoorJasper]

I tried improving readability, also changed how index works slightly (so it's more like how other webserver work, only giving index if it's in publicURL or a sub-directory of publicURL. Would also prevent issues with the future naming conventions inside parcel and any issue with parcel not giving an asset but the index page)

[davidnagli]

👍

[devongovett]

This one seems to break going to e.g. /foo when --public-url=/. In this case, the index HTML file should be served. This is so SPAs that do client side routing can work correctly. Going to merge #974 instead which solves the same issue, but still allows virtual URLs to work correctly. Since Parcel never generates extensionless files, I think this is safe.

[devongovett]

Also changing the default public url to / in #1040.