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

Remove charset=utf-8 default #223

Closed

Conversation

BigBlueHat
Copy link
Contributor

Fixes #220

This also fixes image loading issues when using fetch() + createImageBitmap() in Firefox:
http-party/http-server#296 (comment)

@codecov-io
Copy link

codecov-io commented Jun 19, 2018

Codecov Report

Merging #223 into master will decrease coverage by 0.19%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #223     +/-   ##
=========================================
- Coverage   75.86%   75.67%   -0.2%     
=========================================
  Files           9        9             
  Lines         518      518             
  Branches      117      117             
=========================================
- Hits          393      392      -1     
  Misses         45       45             
- Partials       80       81      +1
Impacted Files Coverage Δ
lib/ecstatic.js 73.3% <100%> (-0.46%) ⬇️

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 609f1c9...e8ac599. Read the comment docs.

@jfhbrook
Copy link
Owner

Thanks for trying to make ecstatic better! I agree that it's alarming that images break because of this. A few minor concerns:

  1. This would be a major version bump as-is, which is fine in and of itself but I dunno that I'd consider this a patchfix of the main line. If this was feature flagged somehow (or made configurable s.t. you can set your own charsets) I could justify it being a minor, and then flip the defaults for a major later.
  2. You mentioned being interested in upgrading the mime package--would that fix this "better"?

@BigBlueHat
Copy link
Contributor Author

made configurable s.t. you can set your own charsets

I wouldn't want to encourage a blanket charset setting option as that puts us back right where we are (passing charset on all the things).

Maybe an --auto-charset option would do the trick? It could become the default in a future major version and the defaulting to utf-8 would be gone.

You mentioned being interested in upgrading the mime package--would that fix this "better"?

See #220 (comment) :)

@athei
Copy link

athei commented Jun 29, 2018

I also stumbled on this because it prevents Firefox from using wasm streaming instantiation. Why is this a major version bump? We are not breaking the API. It is a bug fix isn't it?

@jfhbrook
Copy link
Owner

One person's bugfix is another's expected behavior 🤷‍♂️

@athei
Copy link

athei commented Jun 29, 2018

Okay :D

So the progress on this is stalling because you need to evaluate if you skip this patch alltogether and just apply #218 instead?

Edit: Okay #218 alone does not fix this. The charset would still be applied.

@jfhbrook
Copy link
Owner

I'm not merging #218, at least not as-is, it's open for discussion and so that someone can take the opportunity to fix the issues outlined in that PR.

FWIW I /believe/ this ticket is still an issue even with something along the lines of #218 merged.

@jfhbrook
Copy link
Owner

jfhbrook commented Jun 29, 2018

Dredged up #44 which was the original issue--I think there are good reasons to specify the charset in at least some cases, hence why this exists in the first place.

@athei
Copy link

athei commented Jun 29, 2018

Yes and this patch delegates that charset decision to the mime library like it should. The charset should still be added to types that need it after that patch.

@jfhbrook
Copy link
Owner

A test that confirms that would be nice!

@jfhbrook
Copy link
Owner

jfhbrook commented Jun 29, 2018

Anyway, just to reiterate what I'm looking for around this and #218 :

  • Some answer to cases where setting charset would be appropriate--either delegating to an autodetect library, or an optional way to override, with tests
  • Some answer to passing in custom types--either a separate library for parsing types files with new mime, or the ability to pass in a custom mimetypes resolver in javascript (punting to the user)

@jfhbrook
Copy link
Owner

I get that you're frustrated, btw, with this long-standing issue that prevents a lot of cool things from working right. The problem is that I don't want to merge casual code, but don't have the time to implement the right thing myself. All I can say is, be the change you wish to see in the world.

@jfhbrook
Copy link
Owner

jfhbrook commented Apr 5, 2019

I rolled these changes into #240, which addresses the concerns I had by exposing a way to set a custom content-type lookup function. Thanks!

@jfhbrook jfhbrook closed this Apr 5, 2019
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.

Wrong behavior of application/wasm mime type
4 participants