provide better defaults for mimetype to extension mapping #81

mx1up · 2023-01-16T14:51:38Z

based off pr #15 , afterwards I realized in the meantime mimeFromExtension had already been added 🙄 but without defaults. I went ahead anyway since @kevmoo indicated PR's are still welcome.
So this PR mainly adds the defaults and custom error handling (orElse and nullable version), while maintaining backwards compatibility.

example default:
image/jpeg now nicely maps to "jpg" instead of "jpe"

I copied firstWhereOrNull from the collection package to avoid a dependency.

fixes dart-lang/tools#411
fixes dart-lang/tools#397

Edit: removed collection dependency

…for loop

…eferred extension defined

…d extension not. for backwards compatibility: add `extensionFromMimeOrNull`, and add an `orElse` param for `extensionFromMime` as the standard behavior results in creating an invalid filename.

…ith multiple extensions. There are quite some, but most of them are legacy, and new mimetypes tend to map to 1 extension (no 3 chars limit anymore), so less useful in the future

mx1up · 2023-01-27T13:09:35Z

@kevmoo , @natebosch could you have a look please? 🙏

natebosch · 2023-02-21T20:21:54Z

lib/src/extension.dart

+
+/// Allow for a user-specified MIME type-extension mapping that overrides the
+/// default.
+void addMimeType(String mimeType, String extension) {


I'd be more comfortable with this PR if it didn't include this API.

Why do we need this one?

hi @natebosch , thanks for having a look

this library does not know all possible file formats and I think it's neither realistic nor a goal of this lib to achieve that. The library also seems to be seeded with quite a random list of formats. So it can be quite a frequent situation in which a user of this library needs a format added. A pull request could be made, but the release cycle is not that fast and it's also a bit of overhead for maintainers.

That's why I think it may be useful to have this method so users don't immediately get stuck.

Similarly, this library already provides addExtension.

The 2nd usecase is one in which the user of the lib has a different preference than the one that is included. For example, if one prefers 'text/plain' to map to 'conf' as it currently does, one can still configure that old behavior.

Sorry for the slow response.

I'm worried about having an addMimeType that only impacts extensionFromMime and doesn't impact lookupMimeType.

We could either make this signature addMimeType(String mimeType, String extension, {List<int>? headerBytes}) and try to make this work with lookupMimeType, or we could consider an alternative name like `addExtensionPreference.

@devoncarew @lrhn WDYT?

Generally, I worry about global mutable state.
This is a kind of registry, where any code can add its own preferences, but they then also affect everybody else.

I'd prefer a structure where you have a Mime object instead of global state and functions. Then you can get your own object, modify it, and pass it around to your own code, without affecting other people's Mime objects.
That's a larger change, though.

I don't think we'll be able to do any refactoring of existing usage.

We could add a MimeRegistry class which can be overridden like this. The top-level APIs would continue to use the global map, the MimeRegistry would fall back on the global map, but it could also store local overrides. @lrhn WDYT about something like this?

@lrhn - Should I work on a PR with an implementation of a MimeRegistry, or do you think we can get by with the top level approach?

We can definitely "get by" with the top-level approach, but it's inherently fragile. I prefer to avoid adding fragile features.

What I'd do, if I had all the time in the world, is to have an immutable registry, and a mutator "builder" type.
The top-level functions should then just forward to the corresponding functions on MimeRegistry.defaultRegistry.

abstract interface class MimeRegistry { static get MimeRegistry currentRegistry => ... (probably something that can be zone overridden) ...; factory MimeRegistry.empty() = ...; int get defaultMagicNumbersMaxLength; String lookup(String path, {List<int>? headerBytes}); String extensionFor(String mimeType); MimeRegistryBuilder get build; MimeRegistry rebuild(void Function(MimeRegistryBuilder) mutator); } abstract interface class MimeRegistryBuilder { // I don't know what you'd want, but something like: /// Adds mime type, if it doesn't already exist. /// Adds [extension] to the extensions for [mimeType]. /// Set the header-recognizer function to [headerRecognizer]. void addMimeType(String mimeType, {String? extension, bool Function(List<int>) headerRecognizer}); void removeMimeType(String mimeType); void addExtension(String mimeType, String additionalExtension); void removeExtension(String mimeType, String extension); // Maybe some inspection functions, but it's a little weird if those are not available on the registry itself. Iterable<String> get allMimeTypes; Iterable<String> allExtensionsFor(String mimeType); bool Function(List<int>)? headerRecognizerFor(String mimeType); }

The builder is building on top of an existing registry, and it can either defer to it, lazily copy from it, or eagerly clone.
I'd make the actual implementations hidden.

But I might just be over-engineering. Always a risk 😉

What I'd do, if I had all the time in the world

What do you think we should do given that we have less time than that?

The same thing! The worry about lack of time for something else afterwards 😁

But the concrete problem we're trying to solve here seems to be solved without the addMimeType, so I'd remove that, not add configurability in this PR, and just give access to the default file-type extensions.

Then, if we still need configurability, we can come back and look at it fresh, and not stall this PR.

removed addMimeType

lrhn · 2023-05-10T13:30:15Z

lib/src/extension.dart

+/// If there are multiple extensions for [mimeType], return preferred extension
+/// if defined, or the first occurrence in the map.
+/// If no extension is found, `null` is returned.
+String? extensionFromMimeOrNull(String mimeType) {


I wouldn't have the "OrNull" on the name here. That's just repeating the type.
It distinguishes it from the function below, but see comment there.

lrhn · 2023-05-10T13:34:16Z

lib/src/extension.dart

+  'video/x-matroska': 'mkv',
+};
+
+/// Lookup file extension for a given MIME type.


The name is a noun phrase, so treat this as a parameterized getter. That includes documenting it using a noun phrase too.
(At least the first sentence, after that it's OK to talk about returning a value.)

/// File extension for a given MIME type, if available. /// /// Returns `null` if [mimeType] has no associated extension. /// If [mimeType] has multiple associated extensions, /// the preferred extension is returned.

Don't mention the map, it's not part of the public API, so a reader won't know what "the map" is.

done (had to look up what a 'noun phrase' is, so not sure if I did right ;) )

lrhn · 2023-05-10T13:35:22Z

lib/src/extension.dart

+      ?.key;
+}
+
+String extensionFromMime(String mimeType, {String? orElse}) =>


Needs documentation.
But I'd just drop this method. The user can easily do .extensionFromMime(myMime) ?? orElse() themselves.
Or .extensionFromMime(myMime)! if they know it's there.

btw, I now remember I did this to preserve backward compatibility..

lrhn · 2023-05-10T13:39:00Z

lib/src/extension.dart

+  if (_preferredExtensionsMap.containsKey(mimeTypeLC)) {
+    return _preferredExtensionsMap[mimeTypeLC]!;
+  }
+  return defaultExtensionMap.entries


I'd just inline that extension method here:

for (var entry in defaultExtensionsMap.entries) { if (entry.value == mimeTypeLC) return entry.key; } return null;

It's far too much (unnecessary) syntactic overhead to add your own firstWhereOrNull extension (takes up nine lines further down) to save, basically, one line of code here. And I personally don't find it easier to read either.

mx1up · 2023-12-13T18:45:51Z

@lrhn I applied all your suggestions, please have another look

lrhn · 2023-12-13T19:25:03Z

Looks fine to me. Could probably tweak the documentation phrasing a little, but that's a never-ending search for perfection, we can always iterate on that.

@natebosch WDYT?

mx1up · 2023-12-13T19:47:32Z

thanks!

about documentation: feel free to make suggestions, love to improve.

I just noticed I forgot to remove some part of the documentation about addMimeType, will do it now

mx1up · 2023-12-13T19:54:18Z

removed addMimeType from documentation

mx1up · 2024-01-30T22:59:19Z

@natebosch @lrhn any updates..?

lrhn · 2023-12-13T19:15:32Z

lib/src/extension.dart

+  'video/x-matroska': 'mkv',
+};
+
+/// The extension for a given MIME type.


(Consider changing "extension" to "file extension". Maybe it's just me having worked too much with "extension types" recently, but "extension" isn't feeling precise enough.)

lrhn · 2023-12-13T19:16:35Z

lib/src/extension.dart

+/// if defined, otherwise an extension chosen by the library.
+/// If no extension is found, `null` is returned.
+String? extensionFromMime(String mimeType) {
+  final mimeTypeLC = mimeType.toLowerCase();


(Avoid abbreviations, so LC could be lowerCase, but I'd probably just do:

mimeType = mimeType.toLowerCase();

and not rename.)

I thought it would be bad practice to overwrite incoming params?
anyway, it turned out the function became so trivial I didn't need the var anymore

lrhn · 2023-12-13T19:17:36Z

lib/src/extension.dart

+
+/// Add an override for common extensions since different extensions may map
+/// to the same MIME type.
+final Map<String, String> _preferredExtensionsMap = <String, String>{


It's not possible for code to add to this map, right?
If so, it proably should be const.

indeed, not anymore ;)

but applying the optimization made it no longer possible

lrhn · 2023-12-13T19:18:40Z

lib/src/extension.dart

+    return _preferredExtensionsMap[mimeTypeLC]!;
+  }
+
+  for (final entry in defaultExtensionMap.entries) {


So the point of this code is that the defaultExtensionMap has multiple extensions mapping to the same mime type, so the reverse lookup can choose the "wrong" (not preferred) extension.

Could we create a new map by reversing the defaultExtensionMap and then overriding the ones that we care about, like:

Map<String, String> _preferredExtensionsMap = {for (var entry in defaultExtensiosnMap.entries) entry.value: entry.key}..addAll({ 'application/vnd.ms-excel': 'xls', 'application/vnd.ms-powerpoint': 'ppt', 'image/jpeg': 'jpg', 'image/tiff': 'tif', 'image/svg+xml': 'svg', 'text/calendar': 'ics', 'text/javascript': 'js', 'text/plain': 'txt', 'text/sgml': 'sgml', 'text/x-pascal': 'pas', 'video/mp4': 'mp4', 'video/mpeg': 'mpg', 'video/quicktime': 'mov', 'video/x-matroska': 'mkv', });

Then we only need to look up in one map (and not do a linear search).

That's a possible optimization, what we have is already better than what was before, so no change required.

…nd avoid the linear search

mx1up · 2024-02-19T18:32:20Z

@lrhn I applied all suggestions, please have a look at all unresolved conversations 🙏

kevmoo · 2024-04-19T23:30:34Z

@lrhn ?

mosuem · 2024-08-26T16:11:22Z

As this repo has moved to https://github.com/dart-lang/tools/tree/main/pkgs/mime, please refile the PR there. Thanks!

mx1up · 2024-08-26T17:26:04Z

@mosuem done ✅

dart-lang/tools#431

mx1up added 19 commits January 5, 2023 18:44

initial code from pr !15

86114bd

update copyright year

9f1ef55

improve lookup: case-insensitive and use firstWhereOrNull instead of …

c9e3e15

…for loop

tests cleanup

0f37a4b

check baseline

f4c4a4f

test to check whether each mimetype with multiple extensions has a pr…

a9ec842

…eferred extension defined

reformat

02b9dcb

apparently, the function already existed :roll-eyes: but the preferre…

1ad29e9

…d extension not. for backwards compatibility: add `extensionFromMimeOrNull`, and add an `orElse` param for `extensionFromMime` as the standard behavior results in creating an invalid filename.

probably better not to enforce a preferred extension for mime types w…

84a8f43

…ith multiple extensions. There are quite some, but most of them are legacy, and new mimetypes tend to map to 1 extension (no 3 chars limit anymore), so less useful in the future

added some preferred extensions for common file formats

1f2ab54

cleanup old tests

fbf8f68

test orElse param for unknown mime type

6600a3f

link to file

6fe7ecb

fix title to be more accurate

d57b9fc

rename to existing function

0d6c519

add standard behavior to make example more clear

bb9fc15

include text/plain mapping again

7280615

remove collection dependency

40107ff

lookupExtension has been renamed to existing extensionFromMime

c384ef3

natebosch reviewed Feb 21, 2023

View reviewed changes

mx1up requested a review from natebosch April 4, 2023 09:14

lrhn reviewed May 10, 2023

View reviewed changes

mx1up and others added 5 commits December 13, 2023 18:59

Merge branch 'dart-lang:master' into issue13_contenttype_to_ext

9ca2e5b

drop non-nullable extensionFromMime variant per review suggestion

76f80f6

doc update: try using noun phrase; don't refer to implementation details

7b2804f

don't use extension method

65c7016

remove addMimeType and leave it for another time..

e8a6ed5

mx1up requested a review from lrhn December 13, 2023 18:45

remove addMimeType documentation

f8642bd

Alienjob mentioned this pull request Dec 22, 2023

Generate name for StorageFile based on its mime type (#726) team113/messenger#756

Merged

14 tasks

lrhn reviewed Feb 6, 2024

View reviewed changes

mx1up added 4 commits February 19, 2024 19:00

be more specific about extension

8acfbe4

review feedback: avoid abbreviations, just overwrite function param

f20c40f

since extensions can no longer by added at runtime, we can optimize a…

2be331f

…nd avoid the linear search

local var no longer needed

60e9a7b

mx1up requested a review from lrhn February 19, 2024 18:32

mx1up added 2 commits February 19, 2024 19:34

update docs

54da123

update docs

d190914

TheHCJ approved these changes Apr 19, 2024

View reviewed changes

mosuem closed this Aug 26, 2024

mx1up mentioned this pull request Aug 26, 2024

[pkg-mime] contenttype to ext dart-lang/tools#431

Merged

provide better defaults for mimetype to extension mapping #81

provide better defaults for mimetype to extension mapping #81

Conversation

mx1up commented Jan 16, 2023 • edited Loading

mx1up commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx1up Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx1up commented Dec 13, 2023

lrhn commented Dec 13, 2023

mx1up commented Dec 13, 2023

mx1up commented Dec 13, 2023

mx1up commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx1up commented Feb 19, 2024

kevmoo commented Apr 19, 2024

mosuem commented Aug 26, 2024

mx1up commented Aug 26, 2024 • edited Loading

mx1up commented Jan 16, 2023 •

edited

Loading

lrhn May 10, 2023 •

edited

Loading

mx1up Dec 13, 2023 •

edited

Loading

lrhn May 10, 2023 •

edited

Loading

mx1up commented Aug 26, 2024 •

edited

Loading