Skip to content

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

Closed

Conversation

ELLIOTTCABLE
Copy link
Contributor

@ELLIOTTCABLE ELLIOTTCABLE commented Jan 27, 2020

This is a WIP; it's a fairly large undertaking, and it's looking to be my first contribution (unless someone merges #2985 first! 🤣). I'm opening this for feedback, any assistance or tips anyone wants to offer, and for people to yell at me before I get too deep into it if I do something stupid!

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,

{
   "package-specs": ["commonjs", "es6"],
   "suffix": ".bs.js",
}

... that we specify the file-extension per -bs-package-output, i.e. lower "suffix" into the individual module-format-objects:

{
   "package-specs": [
      {
         "module": "es6",
         "in-source": true,
         "suffix": ".bs.mjs",
      },
      {
         "module": "commonjs",
         "in-source": false,
         "suffix": ".js",
      },
   ],
}

Further, I propose the following changes to the default behaviour (which are, effectively, the logical conclusion of #2307):

  1. "in-source" builds should default to the .bs.* suffix, out-of-source builds should not;
  2. "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 to bs-library/src/path.js in a CJS build, and bs-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" builds
  • jscomp: 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 remove cmj_case from the .cmj-format entirely, make it non-dependant upon suffix, or separate it into two fields in the .cmj: case and suffix
  • jscomp: (IN PROGRESS) Update Js_name_of_module_id et al. to generate .esm module-names

References:


This fixes #3383, and expands upon the work in #2037.

@ELLIOTTCABLE
Copy link
Contributor Author

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:

https://github.com/ELLIOTTCABLE/bucklescript/pull/1/files

@jfrolich
Copy link
Contributor

jfrolich commented Feb 15, 2020

2. "module": "commonjs" builds should default to *.js, and "es6" builds should default to *.mjs

Disagree. Only for node you might want to have .mjs extensions. For frontend projects you'll always need the .js extension for ES6 (you can also set the default to .js with the type set to module in package.json).

@ELLIOTTCABLE ELLIOTTCABLE force-pushed the per-package-spec-suffix branch from c07dcf0 to e3d6174 Compare February 19, 2020 17:40
@ELLIOTTCABLE
Copy link
Contributor Author

ELLIOTTCABLE commented Feb 19, 2020

@jfrolich can you elaborate? I fully admit that I don't know Everything, Ever; but to the best of my knowledge, .mjs is also acceptable for the browser:

  • V8's documentation
  • aren't most users on the browser end going to be using <script type="module" … anyway?

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. ¯\_(ツ)_/¯

@jfrolich
Copy link
Contributor

@ELLIOTTCABLE: Sorry for my short answer. mjs also works in the browser, but it doesn't look like the frontend community is standardizing on it at the moment. Almost all code that is compiled/bundled does not use .mjs, and also projects like React Native do not use .mjs. Even in the node community it has not really been adopted. Can be in the future, but I think for now it's better to have .js as the default.

@ELLIOTTCABLE
Copy link
Contributor Author

@jfrolich 👍, sounds good to me. I prefer to be forward-looking, but not at the expense of practicality.

@ELLIOTTCABLE ELLIOTTCABLE force-pushed the per-package-spec-suffix branch 3 times, most recently from 1077cd2 to 177dc12 Compare March 9, 2020 00:42
@bobzhang
Copy link
Member

how would people really benefit es6 module?
it is common to see code like this

import {xx} from './xx'

This is not Es6 compilant, so it still needs a bundler

@ELLIOTTCABLE
Copy link
Contributor Author

how would people really benefit es6 module?

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!

it is common to see code like this

import {xx} from './xx'

This is not Es6 compilant, so it still needs a bundler

Several things:

  1. Yes, it's common, but that doesn't mean it's the only way; and everyone has to start somewhere with changing behaviour;
  2. I don't think it's a good idea for us to build a tool that assumes everyone is going to add a separate bundle-step; the JavaScript ecosystem doesn't need more forced-tooling;
  3. and, most importantly: that exact form is absolutely supported by Node.js, as long as you use the .esm file-extension, which is what I'm working here to support. (=

@ELLIOTTCABLE ELLIOTTCABLE force-pushed the per-package-spec-suffix branch 4 times, most recently from b4fd150 to 9f9353f Compare April 10, 2020 08:38
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.
@bobzhang
Copy link
Member

this is already implemented in master, thanks for the pioneering work

@bobzhang bobzhang closed this Oct 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide .mjs support for bsconfig.json suffix
3 participants