gh-43: Fix ESModule incompatibility with node16

[maxilie]

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]

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]

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 👍

[Ic3m4n34]

LGTM

[parkerduckworth]

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]

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:
  

  typescript-client/package.json

  Lines 5 to 7 in f96dbb3

  	"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:
  

  typescript-client/package.json

  Lines 8 to 14 in f96dbb3

  	"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]

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]

@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]

@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]

@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.