-
Notifications
You must be signed in to change notification settings - Fork 4
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
Move to TypeScript #78
base: main
Are you sure you want to change the base?
Conversation
@techfg I think this is most of the way done. Last thing is to sort out the docs, which seem to not be generating correctly. Unsure if it's the differences between |
Thanks for putting this together. Added some comments that should get you to where you want to be code & doc wise. Note that there are still some issues with typedoc/typedoc-plugin-markdown/typedoc-plugin-zod so I included a workaround (using |
Thanks @techfg! I'm not sure I'm seeing comments anywhere though 😢 |
@techfg I've gotten a bunch further, but ran out of time for today. I'm not sure the docs are generating right (Options seems to be problematic) unfortunately. |
src/index.ts
Outdated
type Options, | ||
validateOptions, | ||
mergeCollectionOptions, | ||
} from "./options"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to export the types to ensure they are generated in the docs along with their properties and the property comments
import {
type Options,
type CollectionConfig,
validateOptions,
mergeCollectionOptions,
} from "./options";
export type { Options, CollectionConfig };
src/index.ts
Outdated
|
||
// This package makes a lot of assumptions based on it being used with Astro | ||
|
||
const debug = debugFn("astro-rehype-relative-markdown-links"); | ||
|
||
/** | ||
* @type {import('unified').Plugin<[(import('./options.d.ts').Options | null | undefined)?], import('hast').Root>} | ||
* Rehype plugin for Astro to add support for transforming relative links in MD and MDX files into their final page paths. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In doc generation, once moving to type from interface, the type will be inlined in the docs instead of referenced so want to include an @see
to Options
here:
/**
* Rehype plugin for Astro to add support for transforming relative links in MD and MDX files into their final page paths.
* @see {@link Options}
*/
|
||
/** General options */ | ||
export type Options = z.infer<typeof OptionsSchema>; | ||
interface EffectiveOptions extends Options {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of things here:
- Options should use
z.input
whileEffectiveOptions
should usez.infer
(which is the same asz.output
) - There are still some remaining issues with typedoc/typedoc-plugin-markdown/etc. when using types instead of interfaces. If you change
Options
andCollectionsSchema
to interface and make all the other changes that I recommend, the docs should generate identical to current. However, we wantOptions
to be a type to solve for thehover
issue. So, we use the@interface
typedoc tag as a workaround for now. This is not ideal as the docs will haveOptions
as an interface and we're going to lose the TOC, etc. but the code itself will be correct. I'm going to update the isuses I have filed with typedoc, etc. to cover a few additional situations that I found just now. Once all those issues are addressed, we can eliminate the@interface
workaround.
type OptionsSchemaType = typeof OptionsSchema;
type CollectionConfigSchemaType = typeof CollectionConfigSchema;
/**
* Collection specific options
* @interface
*/
export type CollectionConfig = z.input<CollectionConfigSchemaType>;
/**
* General options
* @interface
*/
export type Options = z.input<OptionsSchemaType>;
interface EffectiveOptions extends z.infer<OptionsSchemaType> {};
EDIT: Per my recent comment, the @interface
workaround should no longer be required and its inclusion can be ignored. The docs should generate correctly with Options
and CollectionConfig
as types assuming all the changes in other comments are implemented.
tsconfig.json
Outdated
"noEmit": true, | ||
"allowImportingTsExtensions": true, | ||
"lib": ["esnext"], | ||
"types": ["node"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple config options to consider:
"strict": true,
"noUncheckedIndexedAccess": true,
Also, it may make sense to configure around es2022
instead of ESNext
to avoid any potential stability issues, however unlikely they may be, since we don't need to be on bleeding edge.
EDIT: strict
is required for the docs to generate correctly when using types instead of interfaces so it must be included.
Sorry, forgot to hit the "submit" review button - still a weird nuance of github in IMHO that you have to do that instead of it just posting the comment. Either way, you should see the comments now, let me know if you don't. I'll take a look at the commits you added today and see if anything pops out but the comments you hopefully see now should get you to where you need to be doc wise. |
@@ -418,10 +418,10 @@ describe("astroRehypeRelativeMarkdownLinks", () => { | |||
}); | |||
|
|||
describe("config option validation", () => { | |||
const runValidationTest = async (context, options) => { | |||
const runValidationTest = async (context, options?) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment actually refers to line 34 above (https://github.com/vernak2539/astro-rehype-relative-markdown-links/pull/78/files#diff-15ddc1bcd6ddf3b9483b554a2e56cad27ca25a7924fcd2e1db0bf2d7a849ccadR34) - GH doesn't allow commenting on unchanged lines :(
The old jsdoc type can be removed. It was wrong to begin with so line 35 should actually be:
function testSetupRehype(options: { currentFilePath?: string } = {}) {
@@ -0,0 +1,4 @@ | |||
import astroRehypeRelativeMarkdownLinks from "./plugin"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to the following to ensure docs generate correctly when using types
// NOTE - The order of exports is very important see https://github.com/Gerrit0/typedoc-plugin-zod/issues/8
export type { CollectionConfig, Options } from "./options";
export { default } from "./plugin";
Great news! Based on input from Gerrit0/typedoc-plugin-zod#8, I was able to successfully generate the docs using types (instead of interfaces)! I did find another issue typedoc2md/typedoc-plugin-markdown#743, but its resolution shouldn't hold up getting this finished and merged. Once that issue is addressed, we can always update deps and push new docs with a TOC. I added a few comments to the PR and edit a couple that should hopefully get you to where we want to be docs wise. Lemme know if you have any issues/questions. Getting close, thank you! |
This PR moves this plugin to use TypeScript. It aims to fix #72 and make the upkeep of types (now jsdoc style + custom declaration files) easier.
TODO:
package.json
Context in #72