josdejong / svelte-jsoneditor

A web-based tool to view, edit, format, repair, query, transform, and validate JSON
https://jsoneditoronline.org
Other
818 stars 108 forks source link

fix: export missing types #304

Closed vinzscam closed 10 months ago

vinzscam commented 10 months ago

Hi, thank you for the v0.18.0 release 🙏

We have noticed some typescript errors when bumping vanilla-jsoneditor to the latest release.

Specifically, there seems to be issues with some missing types which aren't exported anymore.

node_modules/vanilla-jsoneditor/index.d.ts:102:15 - error TS2552: Cannot find name 'JSONPointer'. Did you mean 'JSONPointerMap'?

102     [pointer: JSONPointer]: T;
                  ~~~~~~~~~~~

node_modules/vanilla-jsoneditor/index.d.ts:682:15 - error TS2552: Cannot find name 'JSONPath'. Did you mean 'JSONPath$1'?

682         path: JSONPath;
                  ~~~~~~~~

node_modules/vanilla-jsoneditor/index.d.ts:683:16 - error TS2552: Cannot find name 'JSONValue'. Did you mean 'JSONValue$1'?

683         value: JSONValue;
                   ~~~~~~~~~

I guess the issues were caused by this change: https://github.com/josdejong/svelte-jsoneditor/commit/4d0451e981f3d6bc10ca338ed5c03b5b9a4f51fd#diff-b8b77f5be18cf56dca425b3a5b8e9d2e754dd37fe0e3612b95cd4e9bccda08a5L5

josdejong commented 10 months ago

Thanks for your PR, I see the generated index.d.ts has stripped the type JSONPointer = string alias for some reason, that should be fixed.

With the upgrade to Svelte 4, I decided to not embed nor re-export types like JSONPointer coming from an other library: the dts bundler has a problem and doesn't work with Svelte 4, I couldn't find an alternative. Forced to think about this again I realized that it is not very neat any way to copy and re-export types coming from an other library, and the neat way is to just import them from the original source instead.

Using import type { JSONPointer } from 'immutable-json-patch' though gives compiler warnings, and I didn't yet find a solution for that, so I made a little (internal) workaround. Which apparently has an issue to:

https://github.com/josdejong/svelte-jsoneditor/blob/218562dd0a936646a5d1e1d6ee2e7e132edeb24e/src/lib/types.ts#L115-L117

I don't think the right solution is to export JSONPointer again, instead we can:

  1. Figure out the cause of the compiler warning and find a solution. The warning is "JSONPointer imported from external module "immutable-json-patch" but never used"
  2. Don't use JSONPointer in this particular case and export type JSONPointerMap<T> = { [pointer: string]: T } instead

What do you think?

josdejong commented 10 months ago

I did a bit of debugging. It looks like we're hitting some kind of edge case in TypeScript or so: JSONPointer is just an alias of string, where normally keys of an object can only be string.

I think a good solution would be to use Record here:

export type JSONPointerMap<T> = Record<JSONPointer, T>

I just tried that and it solves the issue. Shall we go with that approach?

josdejong commented 10 months ago

I'll publish this fix right away without awaiting your thoughts to have a stable version published again, I hope you don't mind.

josdejong commented 10 months ago

See e23abbc3690799f5a53600c109630de31f05f953, published now in v0.18.1.

vinzscam commented 10 months ago

I'll publish this fix right away without awaiting your thoughts to have a stable version published again, I hope you don't mind.

that's fine thanks! Yeah I haven't touched JSONPointer = string because of the issue mentioned in the comment.

See e23abbc, published now in v0.18.1.

I've checked e23abbc and we are still missing JSONPath and JSONValue, which are used in some exported types.

josdejong commented 10 months ago

That is correct, you should import those from immutable-json-patch, see changelog:

https://github.com/josdejong/svelte-jsoneditor/blob/main/CHANGELOG.md#0180-2023-08-21

vinzscam commented 10 months ago

That is correct, you should import those from immutable-json-patch, see changelog:

https://github.com/josdejong/svelte-jsoneditor/blob/main/CHANGELOG.md#0180-2023-08-21

That's not my main issue, I am not using those types at all.

The main issue is that index.d.ts seems to be broken, specifically lines 680, 681.

josdejong commented 10 months ago

Ahhh, sorry, now I see. It turns out the ReadonlyValue is still trying to import JSONPath and JSONValue from types.ts. I'll publish a fix asap.

josdejong commented 10 months ago

Can you give v0.18.2 a try Vincenzo?

vinzscam commented 10 months ago

Can you give v0.18.2 a try Vincenzo?

thank you, works like a charm! 🙏

josdejong commented 10 months ago

Good to hear, thanks for the feedback!