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.

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

CommonJs vs ES6 #1351

Open
robmoffat opened this issue Sep 12, 2024 · 8 comments · Fixed by #1452
Open

CommonJs vs ES6 #1351

robmoffat opened this issue Sep 12, 2024 · 8 comments · Fixed by #1452
Labels
api FDC3 API Working Group question Further information is requested

Comments

@robmoffat
Copy link
Member

robmoffat commented Sep 12, 2024

For Background:

@Lecss @kriswest and I built the "combined" @finos/fdc3 module in the FDC3 repo, combining all the other modules. For now, this uses ES6 module structure, rather than the FDC3 rollup approach used originally.

However, the original FDC3 module was commonJS. I'm not sure of the ins and outs of this, so:

  1. Is it backwards incompatible to move to ES6? Or does Javascript correctly handle both?
  2. ES6 import seems like the way we want to go forward is this the right time to change?
  3. If we have an old, legacy CJS module, should we expect people to import the monorepo's submodules e.g. fdc3-context? This seems unlikely to work given the amount of literature around the old version.

I want to open this up for further discussion as I'm not an expert on this.

@robmoffat robmoffat added the bug Something isn't working label Sep 12, 2024
@robmoffat
Copy link
Member Author

@novavi

@robmoffat
Copy link
Member Author

@julianna-ciq

@kriswest
Copy link
Contributor

My limited understanding is that the current module contains both:
https://unpkg.com/browse/@finos/fdc3@2.1.1/dist/
and I would assume its not backwards compatible to move to ESM only... but again I'm no expert on that topic. @julianna-ciq or @thorsent might know more.

@kriswest kriswest added question Further information is requested api FDC3 API Working Group and removed bug Something isn't working labels Sep 12, 2024
@thorsent
Copy link
Contributor

TLDR; either configure rollup to produce both commonjs and esm (if you want to be a good citizen), or just go with esm (you'll be fine)

The approach taken by most libraries these days is to produce both commonjs and esm. Rollup can do this pretty easily (the "main" entry in package.json points to the cjs version, while the "module" entry points to the esm version).

Some libraries have chosen to only support esm (an effort to strong-arm the world into moving forward), so people are learning to deal with it.

In general, commonjs is only necessary for node (and usually only in legacy situations). On the web, browsers already support esm as do all the major bundlers. Node however is very persnickety about esm , requiring either that all modules are esm (via a switch) or that modules specifically identify themselves using the .cjs and .mjs notation to indicate their module type.

We most often see this issue rear its head when building unit tests, since mocha and JEST run in node. Anyone in the world who has incorporated the FDC3 library, or needs to incorporate the FDC3 library, in a mocha suite that isn't configured for esm will have issues (as they will with many other public libraries - the solution usually being to use an older version...)

Typically, if you don't have a commonjs option then you have to go "all in" on esm in node in your unit test pipeline, which means including extensions in your imports:

import "../mymodule.js" instead of import "../mymodule"

This syntax change gives a lot of people heart palpitations, because "mymodule.js" may actually refer to a file named "mymodule.cjs"! I think this is the major reason why we're not all running esm already. Personally, I've found that it's just something to get used to.

@robmoffat
Copy link
Member Author

That's brilliant, @thorsent. Also it agrees with @Lecss 's view that we should "move to ES6 and wait until someone complains".

Copy link
Contributor

If doing so, I think we should do so in the open by getting consensus from the SWG. That'll give anyone with a CJS issue a chance to come forward before the release and for us to work out a solution if one is needed (such as an optional legacy build).

I too doubt most will have issues and few will complain about a smaller module!

@Roaders
Copy link
Contributor

Roaders commented Sep 26, 2024

I am no great expert on ES6 by any means. My main experience is being frustrated when things don't work when a package is only available as an ES6 package!! 😄

In theory it should all just work. As mentioned above most / all bundlers support ES6. It is usually jest or other dev tools that don't work (again as mentioned above in the very helpful comment by @thorsent )

In my opinion to reduce friction of adoption and to give developers an easier life it would be better to publish both commonJs and ES6 in the same package. A lot of popular libraries do this and as we're hoping to become a popular library as well I think we should do the same!

@kriswest
Copy link
Contributor

We currently publish a combined package containing both CommonJS and ES6 modules - so this is a proposal to stop doing so.

As discussed at today's SWG meeting, we'll drop this from the 2.2 scope for the time being, while the maintainers review the options and work to create an FDC3 mono repo (which alters the build system and is what prompted this proposal to be raised).

FYI The current rollup config that creates both module types can be found at: https://github.com/finos/FDC3/blob/main/rollup.config.js

@kriswest kriswest removed this from the 2.2 candidates milestone Sep 26, 2024
@robmoffat robmoffat linked a pull request Nov 28, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api FDC3 API Working Group question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants