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

Update mime #154

Merged
merged 1 commit into from
Oct 28, 2018
Merged

Update mime #154

merged 1 commit into from
Oct 28, 2018

Conversation

devongovett
Copy link

#151 but with corrected history

@devongovett devongovett mentioned this pull request Jan 5, 2018
@dougwilson
Copy link
Contributor

Thanks! We'll discuss this in the coming week to see when a new minor version will get cut 👍

devongovett added a commit to parcel-bundler/parcel that referenced this pull request Jan 5, 2018
@dougwilson
Copy link
Contributor

dougwilson commented Jan 5, 2018

We should first evaluate the impact of the changes to the mime types this introduces (like our usual assessment to the mime types). Here is the diff (only changed ones):

-bdoc   application/x-bdoc
+bdoc   application/bdoc
-bmp    image/x-ms-bmp
+bmp    image/bmp
-deb    application/x-debian-package
+deb    application/octet-stream
-dll    application/x-msdownload
+dll    application/octet-stream
-dmg    application/x-apple-diskimage
+dmg    application/octet-stream
-exe    application/x-msdownload
+exe    application/octet-stream
-iso    application/x-iso9660-image
+iso    application/octet-stream
+jp2    image/jp2
+jpf    image/jpx
+jpg2   image/jp2
-jpm    video/jpm
+jpm    image/jpm
+jpx    image/jpx
-m4a    audio/x-m4a
+m4a    audio/mp4
-msi    application/x-msdownload
+msi    application/octet-stream
-org    text/x-org
+org    application/vnd.lotus-organizer
-pdb    application/x-pilot
+pdb    application/vnd.palm
-prc    application/x-pilot
+prc    application/x-mobipocket-ebook
-ra     audio/x-realaudio
+ra     audio/x-pn-realaudio
+raml   application/raml+yaml
-rtf    text/rtf
+rtf    application/rtf
-ttc    application/x-font-ttf
-ttf    application/x-font-ttf
+ttc    font/collection
+ttf    font/ttf
-wav    audio/x-wav
+wasm   application/wasm
+wav    audio/wav
-wmz    application/x-msmetafile
-woff   application/font-woff
-woff2  application/font-woff2
+wmz    application/x-ms-wmz
+woff   font/woff
+woff2  font/woff2
-xml    text/xml
+xml    application/xml

Looks like there is a high level of impact potentially here to existing servers, more than I have ever seen in a mime upgrade. Things like exe, deb, iso, msi, xml, rtf, wav, bmp looks like the largest changes.

devongovett added a commit to parcel-bundler/parcel that referenced this pull request Jan 16, 2018
* add support to rust/wasm

* use child-process-promise instead of async-child-process

* use parent extension instead of fromHtml flag

* create WasmAsset

* RustAsset refactor
 get rid of wargo
 use wasm32-unknown-unknown

* fix lint errors, remove toml package and delete jsconfig.json

* Inline wasm into JS, instantiate, and return exports

* Split bundle loaders into separate modules

Register custom loaders with `bundler.addBundleLoader`

* Split out build queue logic into its own class

Enables `bundler.getAsset` method to resolve and process an asset at any time, outside of the normal asset tree construction.

* Only add a single asset to bundle if raw packager is used

This gives us a separate bundle for each output file.

* Include used bundle loaders, and preload external modules

Allows synchronous import to preload modules in external files, e.g. .wasm file, prior to execution of the JS bundle.

Also processes the HMR runtime like other assets.

* Register wasm loader

* Replace dedicated WASMAsset with RawAsset

No longer returns a URL to the JS bundle if there is a bundle loader defined. This will cause the asset to be preloaded prior to JS bundle execution.

* Update tests

* Clean up bundling code

* Hopefully fix test in travis

* wasm tests

* Define correct wasm mime type

Until pillarjs/send#154 is merged.

* Use WebAssembly.instantiate instead of constructor

Should fix error “WebAssembly.Instance is disallowed on the main thread, if the buffer size is larger than 4KB”

* Fix test

* Fix PromiseQueue bug

* Remove rust for now. Will be added in a separate PR.
@castilloandres
Copy link
Contributor

castilloandres commented Jan 16, 2018

Closed #151 will be following up on this PR 👍 @dougwilson @devongovett

@dougwilson
Copy link
Contributor

So new Express.js minor is looking at Feb release. Ideally this will be included in that release if the mime changes are found to not be impactful to existing servers. If any are, we have time to make changes prior to the minor, so the early it's determined, the better 👍

@robjtede
Copy link

robjtede commented Feb 1, 2018

+1 for this.
WASM support in static servers like browser-sync and lite-server that depend on this down the tree would be amazing.

@dougwilson
Copy link
Contributor

If the change was simply just adding WASM it would have been released already. Instead, it is changing a lot of widely used extensions too. If anyone wants to see it land sooner, please help assess the impact of the changes listed above 👍

@robjtede
Copy link

robjtede commented Feb 1, 2018

Yeah, makes since given this package is so fundamental.

@dougwilson
Copy link
Contributor

So I realized that we took on some changes with mime 1.4.1 because of a security issue, so had to accept the churn in that version. There have been some issues opened in the main Express repo with issues due to that churn, but we didn't want to revert due to security. What I'm getting at is a new comparison between 1.3.4 and 1.6.0 instead of 1.4.1 and 1.6.0. Here is the new comparison of changes types:

-deb	application/x-debian-package
+deb	application/octet-stream
-dll	application/x-msdownload
+dll	application/octet-stream
-dmg	application/x-apple-diskimage
+dmg	application/octet-stream
-exe	application/x-msdownload
+exe	application/octet-stream
-iso	application/x-iso9660-image
+iso	application/octet-stream
-jpm	video/jpm
+jpm	image/jpm
-markdown	text/x-markdown
+markdown	text/markdown
-md	text/x-markdown
+md	text/markdown
-msi	application/x-msdownload
+msi	application/octet-stream
-otf	font/opentype
+otf	font/otf
-ttc	application/x-font-ttf
+ttc	font/collection
-ttf	application/x-font-ttf
+ttf	font/ttf
-wav	audio/x-wav
+wav	audio/wav
-wmz	application/x-msmetafile
+wmz	application/x-ms-wmz
-woff	application/font-woff
+woff	font/woff
-woff2	application/font-woff2
+woff2	font/woff2

All the font changes are definitely known and may or may not cause issues, but I think they are acceptable. Removing those leaves us with the following:

-deb	application/x-debian-package
+deb	application/octet-stream
-dll	application/x-msdownload
+dll	application/octet-stream
-dmg	application/x-apple-diskimage
+dmg	application/octet-stream
-exe	application/x-msdownload
+exe	application/octet-stream
-iso	application/x-iso9660-image
+iso	application/octet-stream
-jpm	video/jpm
+jpm	image/jpm
-markdown	text/x-markdown
+markdown	text/markdown
-md	text/x-markdown
+md	text/markdown
-msi	application/x-msdownload
+msi	application/octet-stream
-wav	audio/x-wav
+wav	audio/wav
-wmz	application/x-msmetafile
+wmz	application/x-ms-wmz

Further, the two markdown I think are a normal move forward, so removing:

-deb	application/x-debian-package
+deb	application/octet-stream
-dll	application/x-msdownload
+dll	application/octet-stream
-dmg	application/x-apple-diskimage
+dmg	application/octet-stream
-exe	application/x-msdownload
+exe	application/octet-stream
-iso	application/x-iso9660-image
+iso	application/octet-stream
-jpm	video/jpm
+jpm	image/jpm
-msi	application/x-msdownload
+msi	application/octet-stream
-wav	audio/x-wav
+wav	audio/wav
-wmz	application/x-msmetafile
+wmz	application/x-ms-wmz

So we can split those into two groups: ones that are now application/octet-stream: deb, dll, dmg, exe, iso, msi; and ones that changed in general: jpm, wav, and wmz.

My understanding is that jpm wasn't correct in the first place, so that sound then be OK.

Looking at wmz it seems like that extension has meant so many various things over time, so probably whatever it is coming through now is meaningless anyway.

The wav change seems big, though it's just removing the x- classification from the type. Research with browsers seems to be that audio/wav is the better choice to serve under vs audio/x-wav so makes sense.

Now that just leaves all the ones that became application/octet-stream. Just looking around there are no "real" types for any of those in the first place. If this will actually be an issue remains to be seen, since most clients (like web browsers) won't even handle any of those types of files, and simply download + save, as which point the served type is lost. So it does seem fine.

TL;DR through review of the changes looks acceptable so it will absolutely be included in the next minor release 👍

@rotemdan
Copy link

rotemdan commented Jul 13, 2018

Latest version of send (at least as seen through live-server), serves .wasm files with type octet-stream instead of application/wasm, which is required in the standard for streaming WASM modules. Not having the right MIME type causes an error (by design):

If the Response is not CORS-same-origin, does not represent an ok status, or does not match the application/wasm MIME type, the returned promise will be rejected with a TypeError.

Is this PR planned to fix this? Will this be eventually merged?

@dougwilson
Copy link
Contributor

I (or someone who wants to volunteer) just needs to rebase this PR so it is mergable to merge. As for releasing send releases are tied to Express releases. A new Express minor is not yet being prepped since the research on this issue was completed for the mime change impacts, so I haven't rebased the PR myself yet since I don't have a timeline for a new Express minor version this would go into.

@dougwilson
Copy link
Contributor

dougwilson commented Jul 13, 2018

This was noted in some other issues but not in this PR, so I want to add it here if people are find their way here:

The types provided by this module are the default types, not the only possible types. This module exposes the underlying Mime instance you can configure with your own types to add / remove / change them as-needed. This module updates to the latest mime ever minor release and web standards move and the mime module didn't provide the wasm type map at the time of our last minor.

To add this (or any other type you want / need), just add a single line your program:

send.mime.define({ 'application/wasm': ['wasm'] })

A full example of using custom types can be found in the README: https://github.com/pillarjs/send/blob/master/README.md#custom-file-types

@rotemdan

This comment has been minimized.

@dougwilson

This comment has been minimized.

@rotemdan

This comment has been minimized.

@robjtede

This comment has been minimized.

@LucasVos

This comment has been minimized.

devongovett added a commit to parcel-bundler/parcel that referenced this pull request Oct 15, 2018
* add support to rust/wasm

* use child-process-promise instead of async-child-process

* use parent extension instead of fromHtml flag

* create WasmAsset

* RustAsset refactor
 get rid of wargo
 use wasm32-unknown-unknown

* fix lint errors, remove toml package and delete jsconfig.json

* Inline wasm into JS, instantiate, and return exports

* Split bundle loaders into separate modules

Register custom loaders with `bundler.addBundleLoader`

* Split out build queue logic into its own class

Enables `bundler.getAsset` method to resolve and process an asset at any time, outside of the normal asset tree construction.

* Only add a single asset to bundle if raw packager is used

This gives us a separate bundle for each output file.

* Include used bundle loaders, and preload external modules

Allows synchronous import to preload modules in external files, e.g. .wasm file, prior to execution of the JS bundle.

Also processes the HMR runtime like other assets.

* Register wasm loader

* Replace dedicated WASMAsset with RawAsset

No longer returns a URL to the JS bundle if there is a bundle loader defined. This will cause the asset to be preloaded prior to JS bundle execution.

* Update tests

* Clean up bundling code

* Hopefully fix test in travis

* wasm tests

* Define correct wasm mime type

Until pillarjs/send#154 is merged.

* Use WebAssembly.instantiate instead of constructor

Should fix error “WebAssembly.Instance is disallowed on the main thread, if the buffer size is larger than 4KB”

* Fix test

* Fix PromiseQueue bug

* Remove rust for now. Will be added in a separate PR.
devongovett added a commit to parcel-bundler/parcel that referenced this pull request Oct 15, 2018
* add support to rust/wasm

* use child-process-promise instead of async-child-process

* use parent extension instead of fromHtml flag

* create WasmAsset

* RustAsset refactor
 get rid of wargo
 use wasm32-unknown-unknown

* fix lint errors, remove toml package and delete jsconfig.json

* Inline wasm into JS, instantiate, and return exports

* Split bundle loaders into separate modules

Register custom loaders with `bundler.addBundleLoader`

* Split out build queue logic into its own class

Enables `bundler.getAsset` method to resolve and process an asset at any time, outside of the normal asset tree construction.

* Only add a single asset to bundle if raw packager is used

This gives us a separate bundle for each output file.

* Include used bundle loaders, and preload external modules

Allows synchronous import to preload modules in external files, e.g. .wasm file, prior to execution of the JS bundle.

Also processes the HMR runtime like other assets.

* Register wasm loader

* Replace dedicated WASMAsset with RawAsset

No longer returns a URL to the JS bundle if there is a bundle loader defined. This will cause the asset to be preloaded prior to JS bundle execution.

* Update tests

* Clean up bundling code

* Hopefully fix test in travis

* wasm tests

* Define correct wasm mime type

Until pillarjs/send#154 is merged.

* Use WebAssembly.instantiate instead of constructor

Should fix error “WebAssembly.Instance is disallowed on the main thread, if the buffer size is larger than 4KB”

* Fix test

* Fix PromiseQueue bug

* Remove rust for now. Will be added in a separate PR.
@dougwilson dougwilson force-pushed the update-mime branch 2 times, most recently from 62ed165 to dcd2ee8 Compare October 28, 2018 00:25
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

rebased

@dougwilson dougwilson merged commit 4645e8b into pillarjs:master Oct 28, 2018
KrysKruk pushed a commit to neftjs/parcel-bundler that referenced this pull request Jun 17, 2019
* add support to rust/wasm

* use child-process-promise instead of async-child-process

* use parent extension instead of fromHtml flag

* create WasmAsset

* RustAsset refactor
 get rid of wargo
 use wasm32-unknown-unknown

* fix lint errors, remove toml package and delete jsconfig.json

* Inline wasm into JS, instantiate, and return exports

* Split bundle loaders into separate modules

Register custom loaders with `bundler.addBundleLoader`

* Split out build queue logic into its own class

Enables `bundler.getAsset` method to resolve and process an asset at any time, outside of the normal asset tree construction.

* Only add a single asset to bundle if raw packager is used

This gives us a separate bundle for each output file.

* Include used bundle loaders, and preload external modules

Allows synchronous import to preload modules in external files, e.g. .wasm file, prior to execution of the JS bundle.

Also processes the HMR runtime like other assets.

* Register wasm loader

* Replace dedicated WASMAsset with RawAsset

No longer returns a URL to the JS bundle if there is a bundle loader defined. This will cause the asset to be preloaded prior to JS bundle execution.

* Update tests

* Clean up bundling code

* Hopefully fix test in travis

* wasm tests

* Define correct wasm mime type

Until pillarjs/send#154 is merged.

* Use WebAssembly.instantiate instead of constructor

Should fix error “WebAssembly.Instance is disallowed on the main thread, if the buffer size is larger than 4KB”

* Fix test

* Fix PromiseQueue bug

* Remove rust for now. Will be added in a separate PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants