-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
fix: do not wrongfully match application/javascript mime #49090
Conversation
Review requested:
|
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.
If I remember correctly, this is because browsers accept this obsolete MIME type. If you can demonstrate that browsers have dropped support, then we can consider it; but if they permit it then we should too.
The current changes do more than just remove support for application/javascript, it also makes the text/javascript check much more strict by no longer allowing the charset=utf-8 mime type parameter. |
@mscdex correct, I'll fix that if needed. Shouldn't it be accepted for all other mime type for ESM loaders like @GeoffreyBooth if browser support is what matters and not the spec, all of the following should be accepted as valid scripts even if obsolete or never written in any spec:
They all work on my Chrome 115.0.5790.171 and Firefox 116.0.2 installation for loading ESM. |
Work as headers, or work as MIME types for |
We would need to do a full deprecation cycle before we can remove this. I would recommend to follow the path of least resistance, which is to keep accepting both, and not add any other one (i.e. the status quo). |
To give more insights, Deno seems to accept the following when importing
Currently, after trying it on Chrome, Firefox, Deno, Node.js already seems the one who restrains the most the accepted MIME type for ESM loading using For what it worth, the behavior I actually support here, accepting only PR: #36328
For now, going through deprecation cycle don't bother me if I can get some guidance about to do it (maybe it's documented somewhere?). I also trust collaborator if you think a full deprecation cycle make sense for a feature that was never documented and never tested. To be honest I see no reason to mimic the behavior of browsers over mimicking the behavior of Deno and inversely. I see no reason either to arbitrarily choose between every MIME type used standardly or non-standardly to pass JavaScript code during the last 20 years. Finally, I see no reason to keep voluntarly supporting an obsolete for at least a year MIME type voluntarly, especially a MIME type that, again, according to the doc, nobody is supposed to use to load ES modules through data url in Node.js. |
The reason is so that code written for one platform hopefully runs in another. The spec is a guide to try to get all the runtimes on the same page; but what the runtimes actually do is the real, de facto standard that we should follow. Our users don't benefit from Node diverging from the crowd here; we should accept what the others do. |
I'll just ignore the part about the spec being the freaking pirate code in Pirates and go straight to the point: Node is already greatly diverging from the crowd by accepting only 1/3 obsolete MIME types and 0/12 never-specified-but-de-facto-standard MIME types. My stand is that only accepting only the common MIME type as it was initially implemented, according to the test currently written and according to the current Node documentation, is perfectly fine and will be gracefully managed by the major semver. If the agreement was to accept the whole pack of unspecified and undocumented potentially acceptable MIME types to match with what "the crowd" do and thus support the "hopefully runs in another" statement, I could have accepted the stance, but I'm definitely not gonna pretend to have a complete and correct exhaustive list and therefore let more motivated people handle this. |
The previous implementation was accepting both
application/javascript
andtext/javascript
while the doc stands that onlytext/javascript
is matching : https://nodejs.org/api/esm.html#data-importsOn top of that, current notice https://datatracker.ietf.org/doc/html/rfc9239 co-authored by TSC member @MylesBorins and collaborator @bmeck stands that
text/javascript
is now common whileapplication/javascript
and other historically used MIME types are obsolete.This PR is a proposal to drop undocumented and untested
application/json
support. If you think this consideration must be adressed otherwise, I'll edit this PR and correct the documentation accordingly.Also, should I amend the commit to indicate it's a breaking change, even if the "feature" is both untested and undocumented?
Fixes: #48957