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

Compilation failure #2

Closed
DanielReid opened this issue May 31, 2023 · 8 comments
Closed

Compilation failure #2

DanielReid opened this issue May 31, 2023 · 8 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@DanielReid
Copy link

DanielReid commented May 31, 2023

Hi,

On attempting to use your (nice-looking!) library, I get a compilation failure:

node_modules/csv42/lib/types/types.d.ts:2:13 - error TS2456: Type alias 'NestedObject' circularly references itself.

2 export type NestedObject = Record<string, NestedObject>;
              ~~~~~~~~~~~~

It appears that the @ts-ignore you put in your types.ts is not added to the compiled types.d.ts.

Some interesting discussion around this behaviour on the typescript repo: microsoft/TypeScript#38628
Not sure what the best solution would be.

I was surprised by this warning since recursive types are possible in newer Typescript. Changing the definition to
export type NestedObject = {[key: string]:NestedObject}; does work, so I expect there's something special about Record here that's throwing soot in the dinner.

@josdejong
Copy link
Owner

josdejong commented May 31, 2023

Thanks Daan, and thanks for reporting this TypeScript issue.

I think a better definition would indeed be something like your proposal, though the values can be an object or anything:

export type NestedObject = { [key: string]: NestedObject | unknown }

This requires changes at the places where NestedObject is used, and I haven't figured out a neat solution that does not involve a lot of plumbing and type casting just to make TS happy. Help improving this would very welcome!

A workaround to fix keeping // @ts-ignore in place would be great for the time being.

@josdejong josdejong added bug Something isn't working help wanted Extra attention is needed labels May 31, 2023
@josdejong
Copy link
Owner

josdejong commented May 31, 2023

Hmm, it may work out like that, when I trade a // @ts-ignore at the type definition for a couple of them in the implementation. Let me try that out.

@josdejong
Copy link
Owner

The workaround mentioned here works, that was very helpful: microsoft/TypeScript#38628 (comment)

I've published that workaround for the time being in v3.0.4. Can you give that a try Daan?

@DanielReid
Copy link
Author

DanielReid commented Jun 1, 2023

Hi Jos,

That works indeed! As the thread notes this isn't a 100% future-proof way of dealing with it (future Typescript releases might strip jsdocs as well), but I approve of the pragmatic approach if the alternative is a lot of work. Thanks for the quick fix. If I can find some spare time and remind myself I might look into the typing a bit myself, but you know how that goes.

@josdejong
Copy link
Owner

Good to hear it works. It's indeed a shaky solution. I will open this issue again as a reminder to find a better solution soon.

@josdejong josdejong reopened this Jun 1, 2023
@josdejong
Copy link
Owner

I've done some refactoring to improve the TypeScript support, see 9ec74ae

There is no need anymore for the // @ts-ignore. The API now has generics, which makes it easier to define value getters with getValue. The type of NestedObject is changed, it is now mostly used internally and only needed for value setters setValue. And internally I introduced another type MergedObject used inside the collectNestedPaths function. I'm quite happy with the result!

Can you give v4.0.0 a try @DanielReid ?

@DanielReid
Copy link
Author

👍 Works like a charm!

@josdejong
Copy link
Owner

😎 thanks for checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants