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

HTML bundle serving bug. #974

Merged
merged 2 commits into from Mar 22, 2018
Merged

HTML bundle serving bug. #974

merged 2 commits into from Mar 22, 2018

Conversation

ry
Copy link
Contributor

@ry ry commented Mar 9, 2018

When serving HTML bundle with publicUrl="/", allow the server to send static assets other than html.

Fixes #782

@codecov-io
Copy link

Codecov Report

Merging #974 into master will decrease coverage by 0.85%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #974      +/-   ##
=========================================
- Coverage   89.46%   88.6%   -0.86%     
=========================================
  Files          68      68              
  Lines        3341    3440      +99     
=========================================
+ Hits         2989    3048      +59     
- Misses        352     392      +40
Impacted Files Coverage Δ
src/Server.js 92.15% <100%> (+0.49%) ⬆️
src/visitors/fs.js 57.63% <0%> (-22.23%) ⬇️
src/assets/WebManifestAsset.js 59.18% <0%> (-19.39%) ⬇️
src/assets/SASSAsset.js 91.48% <0%> (-8.52%) ⬇️
src/assets/JSONAsset.js 84.61% <0%> (-7.7%) ⬇️
src/assets/LESSAsset.js 93.18% <0%> (-6.82%) ⬇️
src/assets/CSSAsset.js 81.73% <0%> (-4.35%) ⬇️
src/visitors/globals.js 92.3% <0%> (-3.85%) ⬇️
src/WorkerFarm.js 91.93% <0%> (-1.62%) ⬇️
src/Logger.js 96.55% <0%> (-1.15%) ⬇️
... and 7 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 0dc062f...1b77bc1. Read the comment docs.

DeMoorJasper added a commit that referenced this pull request Mar 10, 2018
src/Server.js Outdated
@@ -49,6 +50,11 @@ function middleware(bundler) {
} else {
// Otherwise, serve the file from the dist folder
req.url = req.url.slice(bundler.options.publicURL.length);
// If we're serving an html bundle, and the requested path has not file
// extension, serve the index.html. Otherwise go to the file server.
if (bundler.mainAsset.type === 'html' && path.extname(req.url) == "") {
Copy link
Member

Choose a reason for hiding this comment

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

This would break any file that doesn't have an extension?

Copy link
Contributor Author

@ry ry Mar 10, 2018

Choose a reason for hiding this comment

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

Currently only HTML is served, so it wouldn't change the behavior. But it's true that it will not serve files without extensions - I'd argue that is desired behavior for single page apps.

@davidnagli
Copy link
Contributor

@ry @DeMoorJasper Is this a duplicate of #838?

I noticed that #838 closes this PR 🤔 🤷‍♂️

ry and others added 2 commits March 21, 2018 23:28
When serving HTML bundle with publicUrl="/", allow the server
to send static assets other than html.
@devongovett devongovett merged commit 43cab32 into parcel-bundler:master Mar 22, 2018
@devongovett
Copy link
Member

Thanks!

devongovett added a commit that referenced this pull request Mar 22, 2018
Now that this works correctly in the dev server as of #974, this should be a much more sensible default for both production and development builds. Fixes #714.
devongovett added a commit that referenced this pull request Mar 22, 2018
Now that this works correctly in the dev server as of #974, this should be a much more sensible default for both production and development builds. Fixes #714.
devongovett pushed a commit that referenced this pull request Oct 15, 2018
devongovett added a commit that referenced this pull request Oct 15, 2018
Now that this works correctly in the dev server as of #974, this should be a much more sensible default for both production and development builds. Fixes #714.
devongovett pushed a commit that referenced this pull request Oct 15, 2018
devongovett added a commit that referenced this pull request Oct 15, 2018
Now that this works correctly in the dev server as of #974, this should be a much more sensible default for both production and development builds. Fixes #714.
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

5 participants