Build CommonJS module along with ESM and IIFE

[hubertlepicki]

I honestly don't know if this is the best approach but I am quite stuck with trying to upgrade to Phoenix 1.6 on a fairly large project that uses "Jest" for testing of their JavaScript code (elixirforum thread here: https://elixirforum.com/t/phoenix-1-6-0-npm-package-cannot-be-imported-in-jest/42670/4)

I tried various hacks and third party tools suggested on Jest thread but I wasted a whole day and fixing one thing breaks another, typical Node.js developer story. So, I decided to build CJS package of phoenix.js that Jest can understand like it used to up to 1.5.9.

Now I suspect there was a good reason for not including CJS file, but maybe it's just an omission - for the later case I provide this PR below. Note: in this project "phoenix" is the only package that doesn't provide CJS fallback, many other packages do ship with CJS and EMS and I tried to follow the pattern here.

esbuild can build CJS modules as well as ESM, and for the people using
build tools / or testing tools (such as Jest) that do not yet support
ESM modules, it is useful to have fallback when ESM is not availalbe.

This PR includes command to build CJS module along with currently
supported types, and adds entry "main" in package.json to point towards
that file.

This has been tested with Jest, which currently has very poor support
for ESM modules and allows for including "phoenix" in tests and also
stubbing it if needed.

[mveytsman]

This looks good by me, although the two compiled files in priv/static should be taken out of this PR and built as part of the release cycle.

[hubertlepicki]

@mveytsman just removed those from the PR.

[hubertlepicki]

So @mveytsman I've been thinking of it and I'm not sure if this should be built as part of release cycle or earlier - when this PR is being merged. If we did the later, then the CJS file could be used intermediately by folks pointing to GH repo master branch, and that'd be some extra testing + we have copies of the other versions of phoenix.js already commited into master as well, so it'd be consistent. Either way it's fine by me.

[mcrumm]

@mveytsman does this fix #4506?

@hubertlepicki The assets will be built when the PR is merged. Max just meant that, as a security measure, the build needs to be done by someone on the Phoenix team :)

[hubertlepicki]

Glancing at it, it may be that these are 3 separate but related issues, and yes, Node.js does not natively support ESM paclages (yet there is experimental flag to turn it on). I think at least second person's issue should be reaolved as I was getting the same error as @jamesvl but with "jest" not "brunch". Webpack handles those natively, and we already upgraded to webpack while ago so it wasn't the issue.

…

On Wed, Sep 29, 2021, 6:22 PM Michael Crumm ***@***.***> wrote: @mveytsman <https://github.com/mveytsman> does this fix #4506 <#4506>? @hubertlepicki <https://github.com/hubertlepicki> The assets will be built when the PR is merged. Max just meant that, as a security measure, the build needs to be done by someone on the Phoenix team :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4512 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAID6QGL4L5BM5I5HVAE43UEM4NHANCNFSM5E5XW44A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[hubertlepicki]

@mcrumm I wonder if I (or you if you want to do it yourself) should prepare a branch to run by those folks and see if their issues are resolved when pointing to that branch? This is, if this PR is not going to be merged to master shortly, otherwise they can just point to master to test.

[mveytsman]

@hubertlepicki yeah I agree - the file doesn't only need to be built on releases. I more wanted to emphasize that we should keep changes to built artifacts out of PRs like this and as part of an action taken by a team member.

@mcrumm I have not tested but I believe that should fix that!

[mcrumm]

Thanks! 💚💚💚