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

fix(middleware): change how mime type option is handled #670

Merged
merged 5 commits into from
Jun 29, 2020

Conversation

knagaitsev
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

I think that the mimeType option should take priority over existing, known mime types. Otherwise, specifying a mimeType for something like html or js will have no effect.

Breaking Changes

Change of mimeType option priority with existing mime types

Additional Info

@codecov
Copy link

codecov bot commented Jun 27, 2020

Codecov Report

Merging #670 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #670   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files           8        8           
  Lines         227      227           
  Branches       70       70           
=======================================
  Hits          225      225           
  Misses          2        2           
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/middleware.js 100.00% <100.00%> (ø)

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 b1bf4b9...103dd7b. Read the comment docs.

content = handleRangeHeaders(content, req, res);
// Content-Type and headers need to be set before checking if
// the file is in the outputFileSystem, as these things should be
// applied to all files that are being served
Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct me if this approach is incorrect, but it seems that headers and content types need to be applied before rejecting a file that is not in the outputFileSystem. This is because files/routes served by webpack-dev-server like localhost:8080/main, or files that are in the user's directory and are being served, are not in the middleware filesystem but should still have the headers and content types applied to them. That is what the previous behavior was. But let me know if this was an intentional breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, looks like regression

@alexander-akait
Copy link
Member

/cc @hiroppy

Copy link
Member

@hiroppy hiroppy left a comment

Choose a reason for hiding this comment

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

lgtm

@alexander-akait alexander-akait merged commit 7fa2c15 into webpack:master Jun 29, 2020
@alexander-akait
Copy link
Member

@Loonride should we do patch release? Or we have more regressions?

@knagaitsev
Copy link
Contributor Author

@Loonride should we do patch release? Or we have more regressions?

@evilebottnawi Not yet, there still may be more that needs to be done. Still working through integrating it on the webpack-dev-server side

@alexander-akait
Copy link
Member

👍

@knagaitsev
Copy link
Contributor Author

@evilebottnawi I think it is good now for a patch release

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