-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
added .mjs extension to JavaScript #3783
Conversation
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.
Thanks for the pull request!
I left one comment to address, but I think we'll be ready to merge after that :-)
@@ -0,0 +1,6 @@ | |||
import bar from './module.mjs'; |
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 prefer sample files from "real" applications under permissive license. These files are later used to train the Bayesian classifier so it's important that they be representative of actual use.
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.
can you defined "real", this PR is pre-emptive to make the transition easier. Most projects are using pseudo-ESM with .js
(and some form of runtime/compile time transform) but .mjs
will be the same content wise. Though I don't see ESM in the samples either way.
Would using the pseudo-ESM files be reasonable even if they are not to spec?
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.
@pchaigno - you don't actually want the bayesian classifier to auto-detect the difference between es5, pre-standard es6, and es6
this is expected to change within the next year, and the difference is a contextual usage knowledge thing, rather than a characteristic of the contained source. the extension effectively reflects whether the surrounding environment is 1) old, 2) from the future, or 3) has transpilers set up. your classifier explicitly does not consider these issues
A real in-the-wild example has been added; LGTM.
Do I need to do anything else here? CC: @pchaigno |
@bmeck No, you've done everything you need to do. :) We just need to wait for a member of GitHub's staff to give their formal approval before we can merge this. |
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.
Thanks for the PR and welcome to Linguist.
@lildude sorry for the ping, but how long does it take for this to be deployed? |
NP. I don't have the bandwidth right now but I plan to make a release early October. |
+1 for an early October release! We're almost half-way through. |
Used for Node.js when parsing in the Module grammar of ECMAScript.
From Node EP: https://github.com/nodejs/node-eps/blob/master/002-es-modules.md#32-determining-if-source-is-an-es-module
An Internet Draft: https://datatracker.ietf.org/submit/status/88872/
Search results: https://github.com/search?utf8=%E2%9C%93&q=extension%3Amjs+NOT+nothack&type=Code
NOTE: there is an older JavaScript dialect that uses this extension outside of the links above. This still aids in reading those files.