nbubna / store

A better way to use localStorage and sessionStorage
MIT License
1.91k stars 109 forks source link

TypeScript Definitions Issue #97

Closed shirblc closed 2 years ago

shirblc commented 2 years ago

Because of the recent change in TypeScript definitions (from namespace to type) in https://github.com/nbubna/store/commit/1ca5d1605bfdd8f1662317e8f7182f91d5075439, the package is now broken for TypeScript users. I've confirmed that store.set() and store.get() work as usually in JavaScript, but because store is now a type in the TS definitions, it can't be used the same way. (Since our project is written in TS, the latest version of store breaks our builds.)

Is there a change to how we're supposed to handle the package in TS? Or is this a bug? Either way, would really appreciate some help here.

Thank you!

nbubna commented 2 years ago

Drat. I tried to test this, but i don't really use store with TypeScript myself. What errors are you getting and do have some instructions on how i can replicate them to test/fix it? Or do you have a patch for the index.d.ts?

nbubna commented 2 years ago

And i don't understand why store would work better as a namespace than a type.

Ali-Hussein-dev commented 2 years ago

I have the same issue!

Ali-Hussein-dev commented 2 years ago

@nbubna

error message

'store' only refers to a type, but is being used as a value here. ts(2693) store is exported twice, once as type and second as default object, and that is what confuses Typescript I think! if you just rename the store type e.x "storeT" will solve the issue probably!

shirblc commented 2 years ago

@Ali-Hussein-dev Yeah, that's the issue. Where do you see the replication?

@nbubna You can reproduce it by installing TS and store (in any empty folder) and adding this:

import store from 'store2';
store.get('something');

To a script. As soon as you try compiling it, it'll error.

I think it might be happening because the functions were declared in the top level of the namespace, which made them accessible to external scripts (Basically namespaces are modules). As types/interfaces they can only be used to describe objects, so to get the same behaviour, we would need to use something like

import store, { storeConstructor } from 'store2';
const myStore: store = new storeConstructor();

Which I don't think is the behaviour you want, but I'm not sure

shirblc commented 2 years ago

I can try sorting out a patch for index.d.ts, but it'll probably have to wait until tomorrow morning. If nobody does it I'll give it a shot

Ali-Hussein-dev commented 2 years ago

I think the problem can be reproduced in a TS environment, with the following compilerOptions

  "compilerOptions": {
    "moduleResolution": "node",
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "importHelpers": true,
    "target": "es2015",
    "module": "esnext",
    "lib": ["es2017", "dom", "es5"],
    "skipLibCheck": true,
    "skipDefaultLibCheck": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
  }

I changed the name of the store type in the definitions file the error went away.

// ...code
export type storeT = StoreBase & {
  local: StoreBase;
  session: StoreBase;
  page: StoreBase;
};
export default store
shirblc commented 2 years ago

But there's no other "store" - how does it work?

Edit: @Ali-Hussein-dev I've tested your solution but it doesn't work. Have you made any other changes to the TS declarations file?

shirblc commented 2 years ago

@nbubna I've created a very basic reproduction, in case you still need it: https://github.com/shirblc/store2-repro

Ali-Hussein-dev commented 2 years ago

But there's no other "store" - how does it work?

Edit: @Ali-Hussein-dev I've tested your solution but it doesn't work. Have you made any other changes to the TS declarations file?

no the just the renamed the store type

Ali-Hussein-dev commented 2 years ago

@nbubna I've created a very basic reproduction, in case you still need it: https://github.com/shirblc/store2-repro

I do not know why but your tsconfig is different from the one I shared!

shirblc commented 2 years ago

I see, you're counting on

    "skipLibCheck": true,

to prevent errors, but that doesn't solve the issue, it just means the library isn't tested, which essentially has the same effect as turning it into JS.

Ali-Hussein-dev commented 2 years ago

I see, you're counting on

    "skipLibCheck": true,

to prevent errors, but that doesn't solve the issue, it just means the library isn't tested, which essentially has the same effect as turning it into JS.

no, the only change I made and it removed the error log is the change I made to the definition file!

shirblc commented 2 years ago

Look at your tsconfig. You've got 'skipLibCheck' turned on.

The skipLibCheck solution would probably work for anyone installing the package, but if someone adds TS tests to this repo, they'll fail.

shirblc commented 2 years ago

Anyway, the point is, I think there are two ways to sort this:

  1. Either create a namespace with all the variables, functions and types in it; that way it'll all be accessible to TS properly
  2. Create a function to instantiate the store and make it the default export; all other variables and functions will become object properties/methods

Given that localStorage is a globally accessible static thing, I'm guessing the first is the right way you want for this, @nbubna?

nbubna commented 2 years ago

Ok, so the issue is that the index.t.s is now incorrectly telling TypeScript that 'store' is a type, when it's actually an object. Going back to the namespace pattern is not ideal, though, since it was utterly repetitive. If i change 'export type store ...' to 'export type StoreType ...', is there then a way i can simply declare that there is an object named 'store' that is of the type 'StoreType'?

nbubna commented 2 years ago

`export type StoreType = StoreBase & { local: StoreBase; session: StoreBase; page: StoreBase; };

declare const store: StoreType export default store`

This appears to be syntactically correct, does that work? Sorry, i've done a fair bit of TypeScript, but i'm far from expert at it, and i'm clearly struggling to write a good index.d.ts for a JS lib.

shirblc commented 2 years ago

Based on my knowledge of TS, it should work, but let me try it

shirblc commented 2 years ago

Works exactly as it should, thank you very much @nbubna!

Also, to be fair, I haven't done a lot of declarations writing, so totally understand you there. It's not as easy as it seems sometimes

nbubna commented 2 years ago

Thank you, shirblc, for the test repo! And being willing to test. I think i was able to test too, using your repo. I checked it out was able to get the error, then manually tweak the store2 index.d.ts as i show above in the node_modules directory, and ran tsc again. It seems like it does the trick. So i think we're good. I'll get a bugfix release out asap.

nbubna commented 2 years ago

Ok, 2.13.1 has been published. Thank you, again, for the help with this one! And sorry for the trouble.