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.

Sign up for GitHub

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

Build CommonJS module along with ESM and IIFE #1655

Merged
merged 2 commits into from
Oct 8, 2021
Merged

Build CommonJS module along with ESM and IIFE #1655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d87716c
Build cjs alongside esm and iife, closes #1654
mcrumm Sep 30, 2021
fd35697
build cjs assets
mcrumm Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config/config.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ if Mix.env() == :dev do
config :esbuild,
version: "0.12.15",
module: esbuild.(~w(--format=esm --sourcemap --outfile=../priv/static/phoenix_live_view.esm.js)),
main: esbuild.(~w(--format=cjs --sourcemap --outfile=../priv/static/phoenix_live_view.cjs.js)),
cdn: esbuild.(~w(--format=iife --target=es2016 --global-name=LiveView --outfile=../priv/static/phoenix_live_view.js)),
cdn_min: esbuild.(~w(--format=iife --target=es2016 --global-name=LiveView --minify --outfile=../priv/static/phoenix_live_view.min.js))
end
2 changes: 1 addition & 1 deletion mix.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ defmodule Phoenix.LiveView.MixProject do

defp aliases do
[
"assets.build": ["esbuild module", "esbuild cdn", "esbuild cdn_min"],
"assets.build": ["esbuild module", "esbuild cdn", "esbuild cdn_min", "esbuild main"],
"assets.watch": ["esbuild module --watch"]
]
end
Expand Down
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
"version": "0.16.3",
"description": "The Phoenix LiveView JavaScript client.",
"license": "MIT",
"main": "./priv/static/phoenix_live_view.esm.js",
"module": "./priv/static/phoenix_live_view.esm.js",
"main": "./priv/static/phoenix_live_view.cjs.js",
"unpkg": "./priv/static/phoenix_live_view.min.js",
"jsdelivr": "./priv/static/phoenix_live_view.min.js",
"exports": {
"import": "./priv/static/phoenix_live_view.esm.js"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is right but I wonder if we should be providing a "module" here as well (see https://webpack.js.org/guides/package-exports/#providing-commonjs-and-esm-version-stateless)

Copy link

@petermueller petermueller Oct 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I was thinking the same thing, although I'm not sure whether the "exports" key has priority over the top-level "module" key or not. I've not seen anything stating which to use, just the "export" is more flexible.

TL;DR, it sounds like adding the "exports" top-level along with a "type": "module" is the typical way to support ES modules, and the top-level "module" key I added might not be necessary if we add the "type" key instead.

(notes from my reading and digging into the Yarn 2 PnP docs and source)

Okay, if I understand my initial reading on this it sounds like node will treat .cjs and .mjs different when calling import, and .mjs can't be require'd. Setting the extension to .mjs will tell node and modern browsers how to treat it, but then you have to deal with people running into issues where they're doing early development without a bundler and have web server config to only allow js, css, and jpeg, or similar issues that we wouldn't want to force on new folks just trying to get started

"type": "module" sounds like it is the implicit way to do it, and it gives a hint to node/bundlers to treat the import ending in .js as an ES module (i.e. as a .mjs), while still allowing flexibility.

This makes sense to me why we're doing it this way 🤯

This set of articles seems to summarize rather well:
https://dev.to/bnb/conditional-exports-supporting-both-import-and-require-3ihg

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so my take-away from all of that is that the configuration in this PR is seemingly correct.

providing a "module" here as well

@mveytsman possibly, but i am hesitant to make further changes :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw thanks @felix-starman for the thorough reply!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, we are saying that an alternate version of this change (for the relevant) might look like so:

{
  "type": "module",
  "main": "./priv/static/phoenix.cjs.js",
  "unpkg": "./priv/static/phoenix.min.js",
  "jsdelivr": "./priv/static/phoenix.min.js",
  "exports": {
    "import": "./priv/static/phoenix.esm.js",
    "require": "./priv/static/phoenix.cjs.js"
  }
}

is that correct?

Copy link

@petermueller petermueller Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so?
I did some testing and at least for my purposes, esbuild likes to default to using the "iife" (immediately invoked function expression) format, and wraps the cjs file so it can do an "import" at runtime. I was only able to force esbuild to use the esm.js by setting it as the "main", which is an odd esbuild thing? (evanw/esbuild#840)

Ultimately though, having the "module" at the top level looks like it is potentially helpful in certain scenarios?
The "main": "...cjs.js" works for Yarn2 + esbuild when following the docs, and assuming there is a way to tell esbuild to prefer the esm.js file over the cjs, it worked just fine then too.

From the general gist, it seems like leaving the "module" and just adding the "type" is just fine too?

{
  "type": "module",
  "module": "./priv/static/phoenix.esm.js",
  "main": "./priv/static/phoenix.cjs.js",
  "unpkg": "./priv/static/phoenix.min.js",
  "jsdelivr": "./priv/static/phoenix.min.js",
  "exports": {
    "import": "./priv/static/phoenix.esm.js",
    "require": "./priv/static/phoenix.cjs.js"
  }
}

I thoroughly dislike JS dependency resolution now 😄

Copy link
Member Author

@mcrumm mcrumm Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to the party @felix-starman :) and thank you again for the very thorough response!

I've been reading through the links you shared, as well, as the changelog for esbuild, just to get a better sense of things. I think I have that now, so I will share what I have found :)

After digging through the esbuild changelog entries, most notably those in 0.7.0 and 0.10.0, I finally landed on the esbuild docs for main fields which has the most succinct explanation I have yet seen. I will summarize here:

package.json–

  • "main" - This is for node, so it's expected to be CommonJS (CJS).
  • "module" - This is for ECMAScript modules (ESM), except node uses "type": "module" instead. Also lots of packages incorrectly use module to mean, "code for the browser".
  • "browser" - "allows bundlers to replace node-specific files or modules with their browser-friendly versions." I think this has not yet been proposed for Phoenix packages, but perhaps it should be? 🤔

defaults–

"The default main fields depend on the current platform setting and are essentially browser,module,main for the browser and main,module for node. These defaults should be the most widely compatible with the existing package ecosystem. But you can customize them like this if you want to:"

esbuild app.js --bundle --main-fields=module,main

There is so much nuance here. My understanding is that specifying "exports" runs the risk of someone loading both the ESM and CJS bundles into their code, but esbuild does its best to mitigate that? I also get the impression esbuild ignores "exports" entirely, but I am sure other bundlers will rely on it.

My final takeaways:

  • The changes in this PR are good to go as-is
  • esbuild users can specify the --main-fields flag if they need to change the default
  • We should look at replacing "exports" with the top-level "browser" key, but given multiple reports of these settings working for people, I think we save that for another day :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaving this unresolved for posterity :)

"import": "./priv/static/phoenix_live_view.esm.js",
"require": "./priv/static/phoenix_live_view.cjs.js"
},
"author": "Chris McCord <chris@chrismccord.com> (http://www.phoenixframework.org)",
"repository": {
Expand Down
Loading