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

gh-43: Fix ESModule incompatibility with node16 #46

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

Conversation

maxilie
Copy link

@maxilie maxilie commented Apr 24, 2023

Add compatibility with packages defined as ES Modules.

The Problem:

  • Projects defined as ES Modules (module field or moduleResolution field in tsconfig.json is es16, esnext, es6, etc.) cannot resolve the type of the default export in index.ts because it uses the CommonJS format (exports.default =) which ES Modules don't recognize.
  • If you use the export default ... format that ES Modules recognize, then the type definitions will resolve properly but the import will fail at runtime since weaviate was still built as a CommonJS module.
  • This analyzer lists the specific incompatibilities (credit to @ScripterSugar)

Changes Made:

  • Build the .d.ts type definitions and .js output to the same /dist directory.
  • Build the package as an ES Module (change default export, rename build.js & test.js to .cjs).

Effects:
I'm not sure if this could break things. I ran the tests and have been using this code in my own project, in Typescript. I haven't tried to use it in JavaScript but I won't be surprised if JavaScript users will need to replace their require()s with dynamic import()s and change their .js files to .cjs.

If some people are actually using javascript and you don't want to burden them with dynamic imports, then I'm not sure of a good solution for this. The community is moving over to ES Modules, and the typescript devs don't plan to add support for this problem with default exports.

@weaviate-git-bot
Copy link

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Slack channel?

@maxilie
Copy link
Author

maxilie commented Apr 25, 2023

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.beep boop - the Weaviate bot 👋🤖PS:Are you already a member of the Weaviate Slack channel?

I agree to the CLA 👍

Copy link

@Ic3m4n34 Ic3m4n34 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@parkerduckworth
Copy link
Member

Hey @maxilie we have since made some changes to the way the client is build and distributed. Can you try out v1.3.0 and see if it fits your needs?

@iSuslov
Copy link

iSuslov commented May 12, 2023

Hey @parkerduckworth, I'm sorry but changes in v1.3 don't make it work with modules.

Here are a few thoughts:

  • New field "module": is added to package.json:

    "main": "./dist/index.js",
    "module": "./dist/index.mjs",
    "types": "./dist/index.d.ts",

    This field is not supported by Node.js and support is not planned. The only reason it might work is because bundlers will continue support of this field for backwards compatibility. However neither Typescript nor NodeJs is supporting it, and ES6 imports will fail.

  • "exports" is added to package.json, however it's not used here properly:

    "exports": {
    ".": {
    "require": "./dist/index.js",
    "import": "./dist/index.mjs",
    "types": "./dist/index.d.ts"
    }
    },

    This is a right approach, and this is what NodeJs and Typescript recommends. But lets have a look at what conditions are accepted by default:

"import", "require", "node", "node-addons" and "default"
https://nodejs.org/api/packages.html#community-conditions-definitions

So according to the doc, types is a community condition definition, and should always be included first:

"types" - can be used by typing systems to resolve the typing file for the given export. This condition should always be included first.
https://nodejs.org/api/packages.html#community-conditions-definitions

Now lets go to the typescript documentation and take a look at how this field should be used. From the official typescript documentation:

The "types" condition should always come first in "exports".
It’s important to note that the CommonJS entrypoint and the ES module entrypoint each needs its own declaration file, even if the contents are the same between them.
Attempting to use a single .d.ts file to type both an ES module entrypoint and a CommonJS entrypoint will cause TypeScript to think only one of those entrypoints exists, causing compiler errors for users of the package.

Let me add to the last quote. You've probably seen some packages avoid this rule, for example Firebase SDK has one declaration file for set of conditions and use js file extension for every module system they support. Typescript will consider module system for all js files by looking at "type": ["module" | "commonjs"] setting in package.json. If no setting provided, typescript will assume that you are working with commonjs no matter how you import this module, hence every import is considered to be from commonjs module which will work except default exports. They are main bottleneck here.

Finally

Since you have a default export and your default module system is commonjs you should generate declaration files for both module systems you want to support. Declarations should have different file extensions.
Following good practices, name output files the way it contains module system name. Your package.json should look like this:

...
 "exports": {
    ".": {
      "import": {
        "types": "./dist/index.esm2020.d.mts",
        "default": "./dist/index.esm2020.mjs"
      },
      "default": {
        "types": "./dist/index.cjs.d.ts",
        "default": "./dist/index.cjs.js"
      }
    }
  },
  // fallback for older environment 
  "main": "./dist/index.cjs.js",
  "types": "./dist/index.cjs.d.ts",
  // you can leave it for older bundlers support 
  "module": "./dist/index.esm2020.mjs",
  
...

This solution is not easy to implement because you don't really have .mts files in your project and you can't control file extension when emitting declaration files. Workaround is to duplicate existing declaration file with d.mjs extention.

CC: #43

@maxilie
Copy link
Author

maxilie commented May 12, 2023

Hey @maxilie we have since made some changes to the way the client is build and distributed. Can you try out v1.3.0 and see if it fits your needs?

@iSuslov is probably right, and I appreciate all the details provided. Some of it might’ve gone over my head during my brief look at it, though, so I will try the new v1.3 myself, and will report specific actionable findings on Monday or Tuesday. I haven’t had a chance to work with it yet.

@iSuslov
Copy link

iSuslov commented May 12, 2023

@maxilie don't get me wrong, I was talking about v1.3. Your approach looks good, however weaviate team now uses tsup as a bundler.

PS: I must apologize, I went ahead and created a PR too: #56

@parkerduckworth
Copy link
Member

@maxilie @iSuslov I am totally fine with whichever solution fixes this problem. We can stop using tsup as well, if there are better alternatives.

The only concern is to maintain backwards compatibility with existing cjs projects which use this client. If we can maintain that while making this package type module, then we can go ahead and make this change. Looking forward to your feedback

@maxilie
Copy link
Author

maxilie commented May 16, 2023

@parkerduckworth I can confirm that v1.3.0 fixes my issues, but my setup isn't 100% conventional. I suggest merging @iSuslov's PR since it's based on the latest version and it just tweaks a couple of lines to conform to the typescript docs -- and it seems to add more stability and support for a wider variety project setups than the current v1.3.0.

I don't think the details of my project configurations are relevant, but I'll list them here in case it's helpful...

My First Project:

  • package.json has "type": "module".
  • tsconfig.json compilerOptions: "module": "ESNext", "esModuleInterop": true, "target": "ESNext", "moduleResolution": "node".
  • The project is a dependency package used by other projects, so I don't run it in a NodeJS environment directly.
  • Type definitions resolve correctly and the project builds successfully, BUT I haven't tried running it.

My Second Project:

  • package.json and tsconfig.json are the same as my other project.
  • It uses ts-node with the experimental --esm flag to run my .ts file in NodeJS, which means my dependencies can still work (for now) even though at least one of them (not weaviate) is configured incorrectly.
  • Type definitions resolve correctly and the project successfully runs and connects to the weaviate database.

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.

5 participants