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

Add mjs to application/javascript #88

Closed
wants to merge 1 commit into from
Closed

Conversation

bmeck
Copy link
Contributor

@bmeck bmeck commented Aug 8, 2017

No description provided.

"compressible": true,
"extensions": ["mjs"],
"sources": [
"https://github.com/nodejs/node-eps/blob/master/002-es-modules.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the mime listed on that page, though there is a link to another page saying .mjs should be type application/javascript+module

Can the page be updated to clarify what the mime is or link to a different source that does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It links to tc39/ecma262#322 which rejects the new subtype of +module and can be cross referenced in whatwg/html#558 and step 7 of https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script . I will write up a PR and merge it for the node EP explicitly stating that it needs to be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nodejs/node-eps#61 is approved pending confirmation of what the preferred MIME type should be. Either text/javascript or application/javascript.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, unless it comes from one of our sources, it's standard procedure to collect this before adding to custom. It seems to have been useful, as it sounds like the act of clarifying may have changed what it is going to be. If it does end up landing as text/javascript, please feel free to update the PR and I can merge 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed any preference since standards disagree, any web compatible MIME is the main need so this looks fine.

@bmeck
Copy link
Contributor Author

bmeck commented Aug 16, 2017

Addendum that this has been incorporated into an Internet Draft for being merged into the IANA MIME registry : https://datatracker.ietf.org/submit/status/88872/ . That may take a bit of time to land however.

@dougwilson dougwilson self-assigned this Aug 23, 2017
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.

ma ma ma merged

@bmeck
Copy link
Contributor Author

bmeck commented Aug 23, 2017

<3 , will be watching for release since this is an upstream dep for lots of things

@dougwilson
Copy link
Contributor

Alright, 1.30.0 is being published with the change now 🤞

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.

2 participants