-
-
Notifications
You must be signed in to change notification settings - Fork 258
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
Add jpe,jif,jfif as image/jpeg mime type #291
Open
ianchanning
wants to merge
2
commits into
jshttp:master
Choose a base branch
from
ianchanning:patch-1
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the primary source to be linked fom where these are defined. The MDN site is a secondary source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its frustrating because it's not mentioned on the IANA site, but it is an ISO standard.
This is from the official JPEG site https://jpeg.org/jpeg/workplan.html:
Is the ISO page good enough?
There's also this via the JFIF Wikipedia page: https://www.w3.org/Graphics/JPEG/jfif3.pdf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. I wonder where did the MDN page get it then? Maybe I am confused, but your quotes does not say that JFIF is a file extension, just the abbreviation of JPEG File Interchange Format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can contact one of these standards bodies for clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It clearly is a file extension for jpeg files same as this Adobe page talks about it https://www.adobe.com/creativecloud/file-types/image/raster/jpeg-file.html - but it doesn't seem like it was standardised anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Unfortunately this just isn't the purpose of this module, to be a registry -- it is built to be a JSON version of the three sources listed at the top of the module's README. We can accept other ones that are not in there, but it needs to be very clear standard or like something is broken with our scraping of those three sources. I suggest the best way forward without some kind of confirmation from JPEG itself that someone get it registered in IANA. This module is slowing eliminating all entries not included in those three sources, and this seems contrary to the module's direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally for file extensions, we are looking for a first-party source which clearly states that "file extension X represents mime type Y"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I would like to ideally see if we can get these added to one of our upstream sources or fine some page to link to. I actually emailed JPEG to ask them, since it wasn't clear if you did that or not already. I'll see what they say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see it there is no primary source for even 'jpg' being an official extension. IANA doesn't include it. You include it because Apache does and they also include 'jpe' - but with no reason given.
JPEG don't specify any file extensions, Windows just decided to use jpg to keep it short and everyone else jpeg.
So you don't think you can find a primary source for 'jpg'.
So I doubt we'll get much out of JPEG - I don't think they care about the extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually now that I've seen that
jpe
is included in the Apache list I can remove it from this PR.I'm happy if you want to close this - it makes sense to have a clear definition for what is or isn't included here.