knex / knex-schema-inspector

Utility for extracting information about existing DB schema
MIT License
99 stars 43 forks source link

Move knex-schema-inspector to ESM based distribution #124

Closed rijkvanzanten closed 1 year ago

rijkvanzanten commented 2 years ago

I think this Gist from Sindre and the comments beneath summarize it perfectly

https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

My personal favorite: There's more and more JavaScript runtimes nowadays (node/deno/bun/workers/functions etc). ESM is standard JS, CJS is NodeJS common.

This is a breaking change, and should be released as a major version

rijkvanzanten commented 2 years ago

Another benefit of moving to ESM in the output bundle is that you can prevent TS from adding massive amounts of cruft for CJS/ESM compatibility. I'm seeing about a 25% reduction in size of the dist folder 🎉

gpetrov commented 2 years ago

Please do not move to ESM only yet. Many modules aren't ready yet and specially Electron does not support it yet.

Maybe in an year of so 😃 when the community is ready.

kibertoad commented 2 years ago

My suggestion would be to use one of the bundlers that can create both CJS and ESM bundles automatically. That should satisfy needs of both camps :)

rijkvanzanten commented 2 years ago

Another perspective: https://twitter.com/matteocollina/status/1560658851682168834?t=yyNsYZFH_Lyr4d2yhQeb6Q&s=19

let's talk it through properly :)

I've been following various threads along the same sentiment for a while with great interest. Even in this one (in the replies as well as the main thread) I'm seeing a common line of thinking "Yes, we should move to ESM, but no, don't push people to ESM" which I used to agree with, but is becoming a bit of a chicken-egg problem. We see the same in the other comment replied on this PR:

Please do not move to ESM only yet. Many modules aren't ready yet and specially Electron does not support it yet.

Maybe in an year of so 😃 when the community is ready.

People (like the folks behind Electron) won't put in the effort to support ESM if no push is made to move to ESM from the community in the first place 🤔 It becomes a catch-22:

My suggestion would be to use one of the bundlers that can create both CJS and ESM bundles automatically. That should satisfy needs of both camps :)

Fair compromise! I am a little worried about the maintenance overhead though. I've had quite a lot of headaches recently in trying to support both targets in other packages (mostly around compatibility with various bundlers), which made me decide to just bite the bullet and go full ESM. To me, there's a handful of ways forwards:

  1. Merge this PR, and tell people to deal with it. (Very hostile, I agree that it will drive some users away, basically what Sindre has been doing with his vast pool of libraries)
  2. Merge this PR, and put out an open call for other devs to maintain a dual-build. (Causes some friction at first, but hopefully causes other people to step up and help)
  3. Don't merge this PR, and wait for somebody to come along and develop the dual-build setup
  4. Leave everything as-is, and launch the ESM version as a separate forked library
gpetrov commented 2 years ago

Is not that the Electron guys don't want to support ESM - they do! But is is currently impossible https://github.com/electron/electron/issues/21457#issuecomment-1099904505

rijkvanzanten commented 2 years ago

Is not that the Electron guys don't want to support ESM - they do! But is is currently impossible electron/electron#21457 (comment)

Ohhh but that's talking about ESM as the entrypoint, it's not necessarily talking about not supporting ESM in general!

gpetrov commented 2 years ago

Yes indeed it is more about the main entry point. In renderers using esm is just fine.

rijkvanzanten commented 1 year ago

Closing this as it's clearly a point of contention and not a hill I'm willing to die on.