quadstorejs / quadstore

A LevelDB-backed graph database for JS runtimes (Node.js, Deno, browsers, ...) supporting SPARQL queries and the RDF/JS interface.
https://github.com/quadstorejs/quadstore
MIT License
203 stars 14 forks source link

Fix import error #96

Closed alexkreidler closed 4 years ago

alexkreidler commented 4 years ago

I tried to use the devel branch of node-quadstore, because I need the typescript declarations (#90).

Unfortunately, the module loader ran into some issues with the exports field whenever I tried to import. That resulted in Typescript not seeing any declarations. It seemed I was able to fix it by simply making the paths start with ./

I also changed the main through module fields, although they had no effect on the issue.

I'd appreciate if you could merge so I don't have to rely on a local patch and can just point to the github devel branch for now, and then the 7.0 release when it comes out.

Thanks for all your hard work!

alexkreidler commented 4 years ago

I could also remove and .gitignore the yarn.lock file if you prefer using NPM.

alexkreidler commented 4 years ago

Well, it worked mostly, but then I needed to

import {
    TSRdfQuadArrayResult,
    TSResultType,
} from "quadstore/dist-cjs/lib/types"

Which resulted in not a TS error while coding but rather a runtime error when Node used the exports field and couldn't find any dist-cjs entrypoints. I also tried just lib/ but as you know, that's not in the exports field either. For now in my local copy I just got rid of the exports field.

I propose that we either:

  1. Get rid of the exports field so that anyone can import the right types and classes from anywhere (this would require them to know while writing code whether they'll deploy in a CJS or ESM environment)
  2. Create an exports field that properly maps every directory. However, AFAIK it's not possible to do this and also distinguish between CJS and ESM
  3. Simply re-export every type and class that could possibly be used by an external user from the index.ts of the project

Option 1 is less work for maintainers, a little more for users. Option 3 seems the most user-friendly.

jacoscaz commented 4 years ago

Hello Alex!

Thank you for looking into this and coming up with these alternatives. Version 7.0.0 is getting more and more stable but I haven't really looked into our build products yet.

Automatic resolution to either the ESM or CJS directory is giving me a lot of issues across a variety of tools. What do you think of simply having package.json point to the build-cjs directory alone, dropping the exports property and mentioning in the README.md that there's a build-esm for those interested in using native modules?

Something like using require('quadstore') for the CJS version and import 'quadstore/esm' for the ESM version (assuming import can reference subdirectories, I'm not sure about that).

jacoscaz commented 4 years ago

Simply re-export every type and class that could possibly be used by an external user from the index.ts of the project

By the way, I do agree that option 3. leads to the most user-friendly result. However, I'd rather get to the same place by dropping the exports field rather than by forcing myself and anyone who might follow to export everything that users might use from within the multiple entry points of the package. It smells like an anti-pattern.

jacoscaz commented 4 years ago

Hello again. I've played around a bit with your suggestions. If you can, could you have a look at the broken-exports-fix branch ? I've dropped the exports property from package.json and exported a bunch of types from index.ts, which combined should make things work.

alexkreidler commented 4 years ago

Alright, I just tried it and after building quadstore it works! I can do either

import { TSRdfBinding } from "quadstore"
// or
import { TSRdfBinding } from "quadstore/dist/cjs/lib/types"

and my project compiles.

I think your plan is a good one: remove the exports fields so users can access internal types, and try to export most major types from index.ts, but don't worry too much about every possible item a user could want to access.

Thanks for your help on this; I'm very excited for the new release. In fact, I think there is a way on NPM to designate a next or alpha release that doesn't go out to all users, rather those who want to beta-test it. You may want to consider doing that to get more feedback on the new version.

I'll close this for now. Best of luck!

jacoscaz commented 4 years ago

Hello again - I've just published quadstore@7.0.1-alpha.1 straight from the devel branch, tagging it as alpha to make sure nobody is going to use it unintentionally. Let me know whether you encounter packaging-related issues, I'll be happy to fix them as soon as possible. I might be less responsive over the next couple of weeks (summer break) but I'll do my best to keep up.