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

bug: hover & intellisense do not work for Options #72

Open
techfg opened this issue Dec 10, 2024 · 1 comment
Open

bug: hover & intellisense do not work for Options #72

techfg opened this issue Dec 10, 2024 · 1 comment

Comments

@techfg
Copy link
Contributor

techfg commented Dec 10, 2024

As mentioned here, there are two issues with the Options type the plugin exports which I was finally able to reproduce:

  1. When hovering over it, it does not display the properties
  2. When starting to type a property name, intellisense does not work

Repro: https://stackblitz.com/edit/withastro-astro-tgi5dw6j?file=astro.config.mjs%3AL8

Note

This only occurs in dependent projects, these issues are not present in local development of this plugin directly.

Hover Issue

The reason for this is that Options is an interface and interfaces do not expand on hover (see microsoft/TypeScript#38040). In order to display properties on hover, Options needs to be changed to a type. This would typically be relatively straightforward but doing so creates issues with typedoc as until v0.27.0, typedoc would not link type properties in the docs. Typedoc v0.27.0 does support linking type properties, however there have been some issues with doc generation on types. Most of these have been resolved, however a couple remain:

  1. Resolved - Type for optional array element not hyperlinked typedoc2md/typedoc-plugin-markdown#719
  2. Resolved - Unioned type for function param emits object instead of inline type for type that is based on a typedef typedoc2md/typedoc-plugin-markdown#720
  3. Resolved - property emits object for its type instead of inlined typed typedoc2md/typedoc-plugin-markdown#733
  4. Will be fixed in next patch release - hyperlink not generated for {@link type#property} typedoc2md/typedoc-plugin-markdown#732
  5. Pending Investigation - hyperlink not generated for {@link type#property} TypeStrong/typedoc#2808

Assuming 4 & 5 are addressed, we could migrate Options to a type to support hover since Options shouldn't be extended and doesn't require any of the features of interfaces. There is little benefit in making this change, other than supporting hover, and the Typescript docs actually recommend using interfaces unless a feature of type is needed but opinions on this vary and some, like Total Typescript, recommend the opposite.

Intellisense Issue

The reason for this is that Options relies on importing options.mjs which relies on z.object which is a runtime method which won't run in a declaration file. This results in Options being any. In order to address this, we need to eliminate the runtime requirement.

Our current approach, introduced in #40, starts with the Zod schema and infers the typescript type from it. This was done to eliminate duplication the definition of Options and having a single source of truth/maintenance. Unfortunately, it came with an unintended consequence. ☹️
 
The possible solutions for this are:

  1. Migrate to full typescript (*.ts files) - The benefit of this is that we can eliminate all of the hand-rolled *.d.ts files and also avoid having to define types in jsdoc comments, both of which would significantly reduce maintenance of the library. The downside to this is that the build process would need to invoke tsc to generate the *.js & *.d.ts files and then package.json updated to reference the dist/ output. This isn't a big deal as it should only be one added step to the workflow you already have - these files don't need to be comitted (unless you'd want them to be).
  2. Keep *.mjs but use tsc to generate the *.d.ts files - Similar to above in terms of changes needed to build/workflow. We would still need to use jsdoc comments for types, etc.
  3. Start with a type definition for Options and automate the generation of the zod schema creation - Using a tool like ts-to-zod, instead of starting with the zod schema, we would create the zod schema manually by running the tool. Any change to the type would require that the tool be run to update the zod schema and the output included in the commit. This shouldn't happen too often so things should be fairly static but it does introduce a manual step to maintenance when making any change to Options. The thinking here is that the type would be exported directly instead of the inferred type from zod being exported.
  4. Change options.d.ts to include the z.ZodType directly instead of importing the OptionsSchema const and inferring. This eliminates the runtime requirement of z.object, however maintaining the code for the z.ZodType is much higher maintenance than just using the z.object to create the schema.
  5. Go back to maintaining Options and OptionsSchema separately as it was prior to chore: single source of truth for options #40. Outside of having too maintain in two places, this would not require any other changes to the plugin and/or workflows.

@vernak2539 - Sorry for the long explanation on this! After reviewing all the above, please let me know your thoughts on:

  1. Hover - Assuming the remaining typedoc related issues are addressed, would you prefer to leave Options as an interface or migrate to a type in order to support hover?
    • This would be a one-time change and would not require any changes to build process, etc.
    • If I leaned one way or the other, I'd lean towards changing to type to improve usability but it's not a strong lean
  2. Intellisense - I think we must change this as it does not work at all currently and even if we keep Options as an interface, intellisense should work.
    • I am by no means a typescript expert so if there are other ways to solve this beyond those I listed above, please let me know
    • My recommendation on this, FWIW, is to migrate to full typescript (*.ts) files as outside of adding a build step to the existing workflow, it simplifies a lot of things and reduces overall cost of maintenance of the plugin.

Thanks!

@techfg
Copy link
Contributor Author

techfg commented Dec 11, 2024

@vernak2539 - I put together a quick POC on what it would take to migrate to native typescript. Note that this is just a POC - all the tests pass, docs generate the same as before (except for one small item that seems to be related to TypeStrong/typedoc#2808), plugin works in a separate project, etc. but this should not be considered final as I would need to review tsconfig settings closer and touch up a few other things. While migrating to native TS is my recommended solution to the IntelliSense issue, all the other options are viable so deferring to you for input and decision. Just wanted to see what it would take to migrate and generally speaking, it was pretty straightforward and this does resolve the intellisense issue (the hover issue is not addressed in this POC but would be very easy to adjust to incorporate that as well).

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

1 participant