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

feat!: resolve extension conflicts with mime-score, close #116 #119

Merged
merged 7 commits into from
Aug 31, 2024

Conversation

broofa
Copy link
Member

@broofa broofa commented Dec 12, 2023

Adds the mime-score module logic for resolving extension conflicts.

The published mime-score module is now ESM-only, so I've just copy-pasted the logic here, back-ported to CommonJS.

The new test shows which extension->type mappings change as a result of this PR.

@wesleytodd
Copy link
Member

This indicates it is breaking. Can you help me understand what breaks?

@wesleytodd wesleytodd changed the base branch from master to 3.x August 23, 2024 23:02
@broofa
Copy link
Member Author

broofa commented Aug 26, 2024

@wesleytodd Sorry for the delay on this. 'Was at my cousin's wedding this weekend. :-)

I've updated the PR to get CI checks passing. The initial issue was just some eslint-warnings, however I also updated the code to log how extension->MIME type mappings change as a result of this PR, as that's probably of interest. You can npm test to see that, but here's what it currently shows (with my comments on the basis for the change).

Mime-score logic changes extension->type mappings for the following:

* asc -> application/pgp-signature is now application/pgp-keys
No clear winner.  Both types are IANA registered for `asc` extension.  No clear preference.

* mpp -> application/vnd.ms-project is now application/dash-patch+xml
Valid change.  IANA-registered types beat experimental/vendor types ("vnd.*", "x-*")

* ac -> application/vnd.nokia.n-gage.ac+xml is now application/pkix-attr-cert
Valid change.  IANA-registered types beat experimental/vendor types ("vnd.*", "x-*")

* bdoc -> application/x-bdoc is now application/bdoc
Valid change.  IANA-registered types beat experimental/vendor types ("vnd.*", "x-*")

* wmz -> application/x-msmetafile is now application/x-ms-wmz
No clear winner.  This happens because mime-score prefers shorter type names

* xsl -> application/xslt+xml is now application/xml
No clear winner (both are IANA-registered). This happens because mime-score prefers shorter type names.

* wav -> audio/wave is now audio/wav
No clear winner. This happens because mime-score prefers shorter type names.

* rtf -> text/rtf is now application/rtf
No clear winner.  mime-score prefers "application" over "text" types

* xml -> text/xml is now application/xml
No clear winner.  mime-score prefers "application" over "text" types

Most of these are "good" or at least neutral-at-worst changes. The XSL -> XML change is the only one that seems questionable. I have an idea for improving mime-score to fix that, but it'll take a day or two for me to implement that change. If that's of interest (i.e. if you're inclined to merge this PR) I'd be happy to do that work before this gets merged.

@wesleytodd
Copy link
Member

No worries at all about delays, hope the wedding was fun!

So to make sure I understand, the breaking change here is that we are changing what some mime types resolve to? If so, then I actually think this fits within the discussion here. We decided to consider just the JS api semver but not the data sources for now, in alignment with previous practice. Would love to have your opinion on that discussion.

That said, we have been major version reving alot of these packages for the express v5 release, so luckily we can avoid the problems around this decision by releasing this change along with a major just to be safe (where the primart goal of the major is to drop support for old node versions).

@broofa
Copy link
Member Author

broofa commented Aug 27, 2024

the breaking change here is that we are changing what some mime types resolve to

Not quite. This change only applies to what extensions resolve to. I.e. this won't change the behavior of lookup() (getting MIME type for file name or extension). It only applies to the less-used extension() method (getting default extension for a MIME type).

(FWIW, a quick search of the express codebase seems to indicate it's not using the extension() method)

@wesleytodd
Copy link
Member

FWIW, a quick search of the express codebase seems to indicate it's not using the extension() method

Yep, if it was I had never seen it before, so that is part of why I was not sure what was breaking about it.

This change only applies to what extensions resolve to

So I guess my question stills stands though, if we consider the mime data changing as "not part of our semver guarantee" do we really consider this a breaking change? I think we should land this on the next major branch either way, but it would be nice to make sure we have a good description about this change for folks upgrading.

@wesleytodd
Copy link
Member

Extracted from the latest test run:

Mime-score logic changes extension->type mappings for the following:
* asc -> application/pgp-signature is now application/pgp-keys
* mpp -> application/vnd.ms-project is now application/dash-patch+xml
* ac -> application/vnd.nokia.n-gage.ac+xml is now application/pkix-attr-cert
* bdoc -> application/x-bdoc is now application/bdoc
* wmz -> application/x-msmetafile is now application/x-ms-wmz
* xsl -> application/xslt+xml is now application/xml
* wav -> audio/wave is now audio/wav
* rtf -> text/rtf is now application/rtf
* xml -> text/xml is now application/xml
* mp4 -> video/mp4 is now application/mp4
* mpg4 -> video/mp4 is now application/mp4

I am going to land this and will write up a change log which includes this list.

@wesleytodd wesleytodd merged commit 541f9c5 into jshttp:3.x Aug 31, 2024
10 checks passed
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.

2 participants