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

Hybrid NPM Module for ESM and CommonJS #3

Closed
wants to merge 2 commits into from

Conversation

BruceMacD
Copy link
Collaborator

Update the library to allow the NPM module to be used in both ESM and CommonJS.

This javascript library previously targeted ES modules. This means modern ES projects could use the library but requiring the library in CommonJS would error. This change to the build allows the same library to be used in both ES modules and CommonJS.

ESM:

import { Ollama } from "ollama";

CJS:

const { Ollama } = require("ollama");

@saul-jb
Copy link
Collaborator

saul-jb commented Jan 9, 2024

I am leaning against merging this one - this is a chunk of complexity added just to support a legacy module system. If someone still wants to write a package or add it to an existing one in cjs, they can use the import() function, essentially moving the burden of complexity to their end.

@jmorganca have you got a preference here?

@jmorganca
Copy link
Member

I think we can hold off and leave this PR open for now until there's a few requests for it

@danny-avila
Copy link

danny-avila commented Jan 23, 2024

I don't think CJS adds a ton of complexity if it's just a compiling step?

Would love to see this PR merged. Over 70% of all popular packages are written in it, and it's not going away: https://bun.sh/blog/commonjs-is-not-going-away

@radames radames mentioned this pull request Jan 24, 2024
@saul-jb
Copy link
Collaborator

saul-jb commented Mar 14, 2024

@BruceMacD Just coming back to this and it seems like CJS is still more used than I anticipated (still not sure why there are so many new projects that are using CJS instead of ESM but that is another discussion), if you still think this should be implemented don't let me hold this up, there are a couple other PRs related to this as referenced.

References:

@BruceMacD BruceMacD closed this Mar 14, 2024
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.

4 participants