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

TypeScript Definitions Issue #97

Closed
shirblc opened this issue Dec 20, 2021 · 21 comments
Closed

TypeScript Definitions Issue #97

shirblc opened this issue Dec 20, 2021 · 21 comments

Comments

@shirblc
Copy link

shirblc commented Dec 20, 2021

Because of the recent change in TypeScript definitions (from namespace to type) in 1ca5d16, 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
Copy link
Owner

nbubna commented Dec 20, 2021

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
Copy link
Owner

nbubna commented Dec 20, 2021

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

@Ali-Hussein-dev
Copy link

I have the same issue!

@Ali-Hussein-dev
Copy link

Ali-Hussein-dev commented Dec 20, 2021

@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
Copy link
Author

shirblc commented Dec 20, 2021

@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
Copy link
Author

shirblc commented Dec 20, 2021

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
Copy link

Ali-Hussein-dev commented Dec 20, 2021

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
Copy link
Author

shirblc commented Dec 20, 2021

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
Copy link
Author

shirblc commented Dec 20, 2021

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

@Ali-Hussein-dev
Copy link

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
Copy link

@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
Copy link
Author

shirblc commented Dec 20, 2021

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
Copy link

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
Copy link
Author

shirblc commented Dec 20, 2021

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
Copy link
Author

shirblc commented Dec 20, 2021

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
Copy link
Owner

nbubna commented Dec 20, 2021

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
Copy link
Owner

nbubna commented Dec 20, 2021

`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
Copy link
Author

shirblc commented Dec 20, 2021

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

@shirblc
Copy link
Author

shirblc commented Dec 20, 2021

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
Copy link
Owner

nbubna commented Dec 20, 2021

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 added a commit that referenced this issue Dec 20, 2021
@nbubna
Copy link
Owner

nbubna commented Dec 20, 2021

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

@nbubna nbubna closed this as completed Dec 20, 2021
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

No branches or pull requests

3 participants