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

fix: do not wrongfully match application/javascript mime #49090

Closed
wants to merge 1 commit into from

Conversation

Xstoudi
Copy link
Contributor

@Xstoudi Xstoudi commented Aug 9, 2023

The previous implementation was accepting both application/javascript and text/javascript while the doc stands that only text/javascript is matching : https://nodejs.org/api/esm.html#data-imports

On 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 while application/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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Aug 9, 2023
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a 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.

@mscdex
Copy link
Contributor

mscdex commented Aug 10, 2023

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.

@Xstoudi
Copy link
Contributor Author

Xstoudi commented Aug 10, 2023

@mscdex correct, I'll fix that if needed. Shouldn't it be accepted for all other mime type for ESM loaders like application/json tho?

@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:

  • application/ecmascript
  • application/x-ecmascript
  • application/x-javascript
  • text/ecmascript
  • text/javascript1.0
  • text/javascript1.1
  • text/javascript1.2
  • text/javascript1.3
  • text/javascript1.4
  • text/javascript1.5
  • text/jscript
  • text/livescript
  • text/x-ecmascript
  • text/x-javascript

They all work on my Chrome 115.0.5790.171 and Firefox 116.0.2 installation for loading ESM.

@GeoffreyBooth
Copy link
Member

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 data: URLs, or both? This PR is about only data: URLs, right?

@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 10, 2023
@aduh95
Copy link
Contributor

aduh95 commented Aug 10, 2023

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).

@Xstoudi
Copy link
Contributor Author

Xstoudi commented Aug 10, 2023

Work as headers, or work as MIME types for data: URLs, or both? This PR is about only data: URLs, right?
Both.

To give more insights, Deno seems to accept the following when importing data:url:

  • application/javascript
  • application/ecmascript
  • application/x-javascript
  • text/ecmascript
  • text/jscript
  • text/javascript

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 data: scheme.

For what it worth, the behavior I actually support here, accepting only text/javascript, was the initial intent for Node.js (as reflected in doc and tests) and the change to accept application/javascript didn't seem to be discussed in the changing PR.
I don't know if it was discussed elsewhere.

PR: #36328
Commit: ceadb47#diff-06da79a2469f62bfb7c1fac61435050f80d8b93c3221026c02e423f38063d11fL48

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).

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.

@GeoffreyBooth
Copy link
Member

To be honest I see no reason to mimic the behavior of browsers over mimicking the behavior of Deno and inversely.

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.

@Xstoudi
Copy link
Contributor Author

Xstoudi commented Aug 10, 2023

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.

@Xstoudi Xstoudi closed this Aug 10, 2023
@Xstoudi Xstoudi deleted the fix/esm-formats branch August 10, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIME type overmatch in data URLs
5 participants