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

Node ESM support #671

Open
thekevinscott opened this issue Dec 9, 2022 · 9 comments
Open

Node ESM support #671

thekevinscott opened this issue Dec 9, 2022 · 9 comments
Assignees

Comments

@thekevinscott
Copy link
Owner

We currently bundle the library in CJS for Node. It'd be good to support ESM as well, to enable imports of the upscaler without needing require().

Context: #554 (comment)

@thekevinscott thekevinscott self-assigned this Dec 9, 2022
@thekevinscott
Copy link
Owner Author

I'm going to close this for now.

If ESM support in Node is important to you, please leave a comment.

@thekevinscott
Copy link
Owner Author

@adityapatadia - can you share more details about your project structure? If there's a repo online, that could be helpful to browse.

What version of Node are you running? Is "type": "module" in your package.json? Are you using typescript at all?

@adityapatadia
Copy link

We are running node 18.18 with type:module in package.json and not using TS at all. We have all our projects moved to ESM and not able to use CJS. Hope you would add support for ESM soon.

@thekevinscott
Copy link
Owner Author

Thanks for the info, @adityapatadia. I'll do some research into this and report back.

@thekevinscott
Copy link
Owner Author

thekevinscott commented Nov 21, 2023

This is a tricky problem, and I'd welcome support from the open source community if anyone would like to contribute.


Here's some background.

Tensorflow.js comes as three different packages: @tensorflow/tfjs (browser), @tensorflow/tfjs-node (node + CPU), and @tensorflow/tfjs-node-gpu (node + GPU). UpscalerJS supports these three packages via exports, upscaler, upscaler/node, and upscaler/node-gpu.

UpscalerJS comes bundled in different formats:

  • ESM (browser)
  • UMD (browser)
  • CJS (node + node-gpu)

To support ESM in Node, afaik requires one of two approaches:

  1. Enable "type": "module" in the library package.json
  2. Expose the entry as an .mjs file

The first approach would break the existing CJS use case, which leaves the second approach.

Exposing an ESM mjs file is viable, but mjs files cannot import cjs files. So the exported code needs to all be mjs.

This means adding a bundling step that either rewrites files into mjs format, or rolling up into a single file with Rollup or something similar. I've explored Rollup, but the issue there is I've yet to find a plugin that also rolls up type exports. Let's put that aside for now.

There's also the models.

Models also come in the three formats: ESM, UMD, and CJS. Models also export specific model exports, also available in all three flavors (in addition to an index export that re-exports each model).

All the same challenges apply for models, with this additional wrinkle: the default-model is imported directly by UpscalerJS, which requires that types be present, which brings back the original problem that I've not been able to find an elegant solution for supporting an mjs build with valid types.

Since the default-model requires valid type exports, and since I'd really rather ship with valid types as a good ecosystem producer, I think we should find a general solution that solves the issue for all ESM mjs exports.


While I think this is all solvable, I don't think I will have the time to implement particularly soon.

If someone more familiar with the JS tools landscape would like to have a go at this, I'm specifically looking for the following:

Refactor models/default-model to build the following:

  • ESM, that works in both Node and the browser
  • CJS, that works in Node
  • UMD, that works in the browser

If you cd models/default-model && pnpm build you'll be able to see the full build process as it stands today.

I've got an in-progress branch that implements some of this, and importantly, sets up integration tests for a Node ESM environment (that currently fail): https://github.com/thekevinscott/UpscalerJS/tree/ks/node-cjs

@adityapatadia
Copy link

adityapatadia commented Nov 22, 2023 via email

@egoson
Copy link

egoson commented Jan 16, 2024

I can't use your package, need esm support pls

@Juraj-Masiar
Copy link

Juraj-Masiar commented Mar 1, 2024

I'm a bit confused, when I try to import it using const Upscaler = require('upscaler'); or import Upscaler from 'upscaler';, I'll get error:
Error: No "exports" main defined in C:\Users\juraj\git\SearchPreviews\PreviewServer\node_modules\upscaler\package.json

Is that related to this issue or am I doing something wrong? I have dozens of other packages in my project imported like this and only this one fails.

My Webstorm actually want's to import if from import Upscaler from 'upscaler/dist/node/upscalerjs/src/node';, but it still fails with the same issue.

EDIT:
After checking the exports, I see I need to use import Upscaler from 'upscaler/node'; to import it. And that almost works, except for the new error: Error: Cannot find module '@tensorflow/tfjs-node'.

@thekevinscott
Copy link
Owner Author

@Juraj-Masiar you'll need to install @tensorflow/tfjs-node from npm alongside upscaler. Check out this example which also has a runnable code example.

To the broader question of ESM support: yes, I'd love to have it. I would welcome a PR from the community if someone would like to take a swing at this.

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

No branches or pull requests

4 participants