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

[Feature request] CommonJS compatability #152

Closed
Emsu opened this issue Jun 20, 2023 · 7 comments · Fixed by #545
Closed

[Feature request] CommonJS compatability #152

Emsu opened this issue Jun 20, 2023 · 7 comments · Fixed by #545
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Emsu
Copy link

Emsu commented Jun 20, 2023

CommonJS compatibility
In general, the feature you want added should be supported by HuggingFace's transformers library:

  • If requesting a model, it must be listed here.
  • If requesting a pipeline, it must be listed here.
  • If requesting a task, it must be listed here.

We're hoping in addition to ESM, that transformers.js can compile to both CJS and ESM targets.

Reason for request
Why is it important that we add this feature? What is your intended use case? Remember, we are more likely to add support for models/pipelines/tasks that are popular (e.g., many downloads), or contain functionality that does not exist (e.g., new input type).

It's important for the wider audience of Javascript and typescript developers to be able to use the library especially those working in codebases where it'd be difficult to move everything to ESM.

Additional context
Add any other context or screenshots about the feature request here.

Here's the main error I get when I try to use the package

Error [ERR_REQUIRE_ESM]: require() of ES Module .pnpm/@[email protected]/node_modules/@xenova/transformers/src/transformers.js from local-vectors/lib/embed.js not supported.
Instead change the require of transformers.js in local-vectors/lib/embed.js to a dynamic import() which is available in all CommonJS modules.
    at local-vectors/lib/embed.js:71:95 {
  code: 'ERR_REQUIRE_ESM'
}

I think you can have the package.json output main, module and types for CJS, ESM and TS support.

Here's two reference articles on the web:
https://adamcoster.com/blog/commonjs-and-esm-importexport-compatibility-examples
https://antfu.me/posts/publish-esm-and-cjs

@Emsu Emsu added the enhancement New feature or request label Jun 20, 2023
@kungfooman
Copy link
Contributor

We're hoping in addition to ESM, that transformers.js can compile to both CJS and ESM targets.

The distributed npm package already contains a CJS file, so transformers.js "can compile to both CJS and ESM" already?

Not sure how you run into that error or what your entire build setup looks like, can you share a test repo/setup to inspect?

If you don't use the distributed ES5 bundle but ES6 as it is, you need to use commonjs plugin in rollup via plugins: [commonjs(), nodeResolve()]... but I don't know what you are even trying to do right now.

And there already is a "types": "./types/transformers.d.ts", in package.json for types.

It's important for the wider audience of Javascript and typescript developers to be able to use the library especially those working in codebases where it'd be difficult to move everything to ESM.

In any case, you can already just use the distributed ES5 file and just keep your legacy stack (which I wouldn't recommend though, because it's legacy for a reason and ES6 is better in so many ways - and every browser supports it).

@xenova
Copy link
Collaborator

xenova commented Jun 20, 2023

@Emsu For your use-case, are you not able to use the dynamic import syntax? We wrote a short tutorial for how to use the library with CommonJS imports here: https://huggingface.co/docs/transformers.js/tutorials/node

That said, if it is simple to also distribute the library as a CommonJS module (hopefully only 1-2 files in dist), then I think this is definitely something we can put some time into.

@xenova
Copy link
Collaborator

xenova commented Jun 20, 2023

@hwang251
Copy link

I think this would help fix this issue in LangChain, langchain-ai/langchainjs#2992

@anilbandrapalli
Copy link

anilbandrapalli commented Jan 31, 2024

Hi,
in my poc application i am using direct esm module import , it is working fine. but where as my actual application is ts application using webpack build. even though after using dynamic import also i am facing "ERR_REQUIRE_ESM" issue.

Even i tried using v3 branch but still same issue

Kindly guide us in this

@mschudt
Copy link

mschudt commented May 9, 2024

I think this would help fix this issue in LangChain, langchain-ai/langchainjs#2992

To be specific, this comment langchain-ai/langchainjs#2992 (comment) was the only thing that worked for me out of everything I've tried.

const TransformersApi = Function('return import("@xenova/transformers")')();
const { pipeline } = await TransformersApi;

@dorb-aiola
Copy link

what is the difference between using :

const TransformersApi = Function('return import("@xenova/transformers")')();
const { pipeline } = await TransformersApi;

and directly using :

const { pipeline, env, cos_sim } = await import('@xenova/transformers');

the first one works but the second throw error on runtime with "...dynamic import() which is available in all CommonJS modules"

logically i don't see the difference but for some reason one worked and one did not, both of them are inside "async" function and using CommonJs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants