-
Notifications
You must be signed in to change notification settings - Fork 469
Support .mjs and .bs.mjs for Node.js ES-module-support, and lower "suffix" option into "package-specs", #4116
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
Support .mjs and .bs.mjs for Node.js ES-module-support, and lower "suffix" option into "package-specs", #4116
Conversation
8eb0318
to
c07dcf0
Compare
If you prefer GitHub's "files" view, here's a version of this pull-request rebased on top of the whitespace-and-formatting fixes I'm making along the way: |
Disagree. Only for node you might want to have |
c07dcf0
to
e3d6174
Compare
@jfrolich can you elaborate? I fully admit that I don't know Everything, Ever; but to the best of my knowledge,
That said, I don't want to sidetrack this; and given that "not changing the default" is always a good idea for backwards-compatibility, even having one person have spoken out against doing so makes me tend to think “sure, let's leave it be, then.” To be clear, I'd prefer we future-proof the default while BuckleScript is still small; but it's far from my primary concern here. Feel free to weigh in, or not. ¯\_(ツ)_/¯ |
@ELLIOTTCABLE: Sorry for my short answer. |
@jfrolich 👍, sounds good to me. I prefer to be forward-looking, but not at the expense of practicality. |
1077cd2
to
177dc12
Compare
how would people really benefit es6 module?
This is not Es6 compilant, so it still needs a bundler |
Plenty of ways, but I'd also posit that that's not our concern — instead, it's best to be as compliant as possible, and let people use us in whatever way works best for them!
Several things:
|
b4fd150
to
9f9353f
Compare
These are the portions of the codebase changed in later commits to this branch. I've rebased any whitespace / formatting-only changes back into this commit, to produce a slightly-less-noisy diff.
This gets everything compiling again, but we're far from done! 😫
Additionally involed: - Split `cmj_case` type into new, simplified `leading_case` type and aformentioned `package_info` changes - Store file-extension(s) in `.cmj` files - Bump `cmj_magic_number`, as it now contains new information Backwards-compatibility is maintained.
In addition, add `.bs.cjs` alongside `.bs.mjs` as a cleaning-supported file-extension.
3de2fb8
to
ce8e5c9
Compare
this is already implemented in master, thanks for the pioneering work |
Now that
.mjs
support has been made the default behaviour in Node.js, I think it's time to finally add first-class.mjs
and ES-Module support to BuckleScript.My proposal is thus: that instead of the single, global boolean flag that decides between
.bs.js
and plain.js
,... that we specify the file-extension per
-bs-package-output
, i.e. lower"suffix"
into the individual module-format-objects:Further, I propose the following changes to the default behaviour (which are, effectively, the logical conclusion of #2307):
"in-source"
builds should default to the.bs.*
suffix, out-of-source builds should not;"module": "commonjs"
builds should default to*.js
, and"es6"
builds should default to*.mjs
(With these defaults, the configuration "just works" with all existing JavaScript bundlers I'm aware of (Rollup, Webpack, etc) — i.e.
import ... from 'bs-library/src/path'
will resolve tobs-library/src/path.js
in a CJS build, andbs-library/src/path.mjs
in an ESM build.)Finally, in the process, as suggested by @JohnGurin, I'm removing the hard constraints on the value of
"suffix"
: it can be any string, but any value other than the four described above will print a warning-message. (In particular, the tree-cleaning involved with"in-source" && "suffix": ".bs.js"
builds will only happen for, specifically,".bs.js"
and".bs.mjs"
, not for".bs.mycoolmadeupextension"
.)This will almost certainly have to wait until a major-release, as it unfortunately looks like it involves a slight change to the
.cmj
format.Todo:
(subject to change, of course, as I spelunk the related source-code!)
bsb
: Replace"suffix"
-detection with detection of subfields of"package-specs"
,bsb
: Add backwards-compatible detection and a deprecation notice for root-level"suffix"
bsb
: Update tree-cleaning operation to clean".bs.mjs"
as well, for"in-source"
buildsjscomp
: Remove the-bs-suffix
flag, and instead add a suffix-field to the-bs-package-output
field (amdjs:lib/amdjs:.bs.mjs
)bsb
: Update Ninja-generation to use new-bs-package-output
form instead of-bs-suffix
jscomp
: Either removecmj_case
from the.cmj
-format entirely, make it non-dependant upon suffix, or separate it into two fields in the.cmj
:case
andsuffix
jscomp
: (IN PROGRESS) UpdateJs_name_of_module_id
et al. to generate.esm
module-namesReferences:
This fixes #3383, and expands upon the work in #2037.