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

create exports for cjs and mjs #59

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexspevak
Copy link
Contributor

We are using CJS modules, so we created pull request to solve this issue.

@pastelmind
Copy link
Collaborator

Thanks for the pull request! However, I'm afraid providing a CommonJS bundle is currently out of scope.

Disclaimer: This is my personal opinion and not of our team.

  1. I believe ESM is the right way forward. ESM is the de facto standard input format for bundlers, and is gaining support as an executable format for major web browsers and more recently Node.js.
  2. Providing both ESM and CommonJS bundles is tricky and can lead to the dual package hazard. This is problematic because @webtoon/psd exports class constructors such as Psd and Layer. These can be used with instanceof--which is not recommended, but works.
  3. Even if we did decide to provide a CommonJS bundle, we wouldn't use main for CJS and and exports for ESM. We would prefer conditional exports with the export field. (We currently use the main field to provide an ESM package, but this was an arbitrary choice; we may move to using exports instead of main in the future regardless of whether we provide a CommonJS bundle)

Can you use dynamic import() to import @webtoon/psd in your Node.js project? AFAIK you can use import() in CommonJS code since Node.js 12+.

@pastelmind pastelmind self-assigned this Oct 17, 2022
@pastelmind pastelmind added the question Further information is requested label Oct 17, 2022
@alexspevak
Copy link
Contributor Author

Thank you for your suggestion. However, we are using Typescript for our library and it is not possible at the moment for the Typescript compiler to distinguish between dynamic and regular import. In other words, dynamic import gets compiled to require.
microsoft/TypeScript#43329

@pastelmind
Copy link
Collaborator

Ah, gotcha. That is a vexing problem. Allow me to think more about this.

@pastelmind
Copy link
Collaborator

From the thread, it seems that TypeScript 4.7+ amended the problem with module: "nodenext". You would have to...

  • Use TypeScript 4.7+, and...
  • Set module: "node16" or module: "nodenext" in tsconfig.json

This should prevent the compiler from erroneously transpiling dynamic import() to require() calls. Can you apply these changes to your project, or would it be too disruptive for you?

@alexspevak
Copy link
Contributor Author

Hi,

sorry for the long-awaited answer. We are considering your suggestion concerning dynamic imports. However, this comes short when importing types, since we also use typescript in our library. We are now using a vendor submodule where we can set up node "CJS" module type in package.json, but this has its shortcomings as there can be discrepancies between the original and vendor lib and takes a toll on maintenance. We will be discussing transferring our library to ESM tomorrow.
I am not sure if it's a bug or not, but considering what's written in devblogs you should end imported files with ".js" extension. At least, when we tried to use it using import(), the tsc compiler reported errors about missing file extensions. We had to use flag skipLibCheck when compiling.

@pastelmind
Copy link
Collaborator

pastelmind commented Oct 26, 2022

Ah, I see. It appears that our project itself is not ready for module: "nodenext" after all, since we omit the .js file extensions in import statements throughout our code. Thanks for informing us.

I've since changed my mind on the matter, though. Providing CommonJS bundles for CJS users seems worth risking the dual package hazard. I will open a separate PR that adds a CJS bundle, though I want to use conditional exports instead of the main/exports trick you used.

P. S. Are you building a library or an application? Does your project target Node.js, web browsers, or both? Which version of Node.js do you use? What bundler, if any? These are important information we need to decide on how to best support you.

@alexspevak alexspevak closed this Nov 1, 2022
@alexspevak alexspevak reopened this Nov 1, 2022
@alexspevak
Copy link
Contributor Author

We have decided to switch to ESM, but CommonJS can be also useful for us.

As for your questions: we are building a library which will be used for both node.js and the browser. We are using 16.3.2+ Node and for building, we use vite when bundling onto the web (we don't bundle the library itself, we bundle the web build of the app which uses it as a dependency).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants