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

Implement TypeScript type generation for the configuration #1014

Closed
wants to merge 3 commits into from

Conversation

Lehoczky
Copy link

Resolves #1004

📚Description

This PR adds a new script to the project: schema/build-config-declaration.js. The script takes the existing configuration schema, and generated a new file markdownlint-config.d.ts with type declaration for the config. This script now runs as part of the build-declaration npm script.

The generated type is then used in the libs/markdownlint.js file to enrich the existing Configuration type:

/**
 * Rule configuration object.
 *
- * @typedef {Object.<string, RuleConfiguration>} Configuration
+ * @typedef {import("../schema/markdownlint-config").Config} Configuration
 */

With this change, the functions that already used this type now has proper suggestions:

image

A new defineConfig() function was added as well to make it easier to define configurations in js files, like vite, astro or nuxt does.

Comment on lines +56 to +63
{
"files": [
"schema/*.js"
],
"rules": {
"unicorn/prefer-top-level-await": "off"
}
},
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because these are commonjs files, we cannot use top-level imports in them.

Comment on lines +1308 to +1313
/**
* Rule configuration object.
*
* @typedef {boolean | Object} RuleConfiguration Rule configuration.
*/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are no longer using this type as part of the Configuration, I moved it next to the only type definition that uses it.

@DavidAnson
Copy link
Owner

Thanks for sharing this!

I like what it does for the user and the way the new types are referenced is clean, but build-config-declaration.js is a lot more complicated than I'd like. Per the discussion in #1004, I'm hoping for something simpler. I'll need to spend some time exploring options here before I can review this PR. I have a few other tasks on the TODO list, so it may be a week or two before I come back to this.

I don't think I'd suggest spending time on this before I do, but if you're bored I can say after a first pass that defineConfig would need to be documented in README.md and type-check.ts should have a line or two added to validate this. I'd also rather not touch the files in the micromark directory, as that seems unrelated.

@Lehoczky
Copy link
Author

Let me know when you think we can move forward with this approach, and I will update the readme and the type checks^^

Regarding the changes in the micromark folder, I did those, because the linting would fail otherwise

DavidAnson added a commit that referenced this pull request Oct 23, 2023
@DavidAnson
Copy link
Owner

I spent a bit of time this evening and pushed the branch condef to show the approach I imagined and was hoping for in #1004 - you can see the diff here: next...condef

I only need to write a few lines of code to work around a quirk/bug of TypeScript index types. All the type information comes directly from the JSON schema. From what I can tell experimenting quickly in type-check.ts around line 85, the definition is working as intended. I didn't spend time on defineConfig since you'd already done that in this PR.

The only thing I see missing vs. what's proposed in this PR is deprecations - but that's part of the 2019-09 JSON specification and may carry over automatically once used/added.

@Lehoczky
Copy link
Author

There are a few key differences with these approaches.

JSDoc quality

Even though the generation in this PR is longer, most of the lines are for making the JSDoc comments more readable and neatly organized. For example, here is the MD021 rule:

image
image

The upper one has the description, the most important part first, has a paragraph for the aliases and a link to the rule's documentation.

Readability of the d.ts file

I would say the generated file is more readable in this PR. If we compare the MD022 rule for example:

image
image

The second interface has a lot of duplication and is bit weird. The upper is short and readable.

Interfaces are also randomly generated in the condef branch's output. Sometimes there is an interface for a rule, sometimes the types are inlined:

image

Errors

When an interface is generated for a rule in the condef branch's output, there is no JSDoc for that rule when using the Configuration type. This happends for the MD022 rule for example:

image
image

Compromises

We can delete most of the code from the generation script, if we don't want to make the JSDoc comments more readable, and the output would still be okay. I think it it's a nice addition, but if you don't think it's worth the ~100 plus lines of code, I can delete it.

@DavidAnson
Copy link
Owner

JSDoc quality: I agree that what you show is richer. At the same time, the JSON schema is seen by more people and if there are readability improvements to be had there, I’d prefer to make them so everyone benefits. The rule link is a nice touch. Good news is that it seems VS Code automatically linkifies bare URLs in the description field, so I’ve made myself a note to add that to each rule’s description in the JSON schema.

Readability of the .d.ts file: This is not a goal in my mind; that file is not something users are expected to read. I agree it’s nice if it’s more readable, but I would not base a decision on this unless it was the only difference.

Errors: This seems like a VS Code problem? It’s worth looking into on my part.

FYI, I’m iterating on a different PR at the moment and will come back to this when that’s done.

@DavidAnson
Copy link
Owner

Note to self

Fix for the problem raised above in the section "Errors". As it happens, this also largely addresses (and improves upon?) the section "Readability of the d.ts file" by defining everything inline.

diff --git a/schema/build-config-schema.js b/schema/build-config-schema.js
index 9a13fd7..a6290b5 100644
--- a/schema/build-config-schema.js
+++ b/schema/build-config-schema.js
@@ -530,9 +530,7 @@ for (const rule of rules) {
     scheme.additionalProperties = false;
   }
   for (const [ index, name ] of rule.names.entries()) {
-    schema.properties[name] = (index === 0) ? scheme : {
-      "$ref": `#/properties/${rule.names[0]}`
-    };
+    schema.properties[name] = scheme;
   }
 }

@DavidAnson
Copy link
Owner

I spent about an hour playing with quicktype this evening. It seems very similar to json-schema-to-typescript in a lot of ways. At one point, I thought I found a command-line I could use instead of writing any code, but there are some of the same issues with this library:

  • configuration.d.ts is full of red squiggles unless the default handling of additionalProperties is altered. Link to the json-schema-to-typescript bug for this: Broken generation with additionalProperties bcherny/json-schema-to-typescript#356. I couldn't find a corresponding bug for quicktype, but the same edit of the .d.ts fixes the squiggles: [property: string]: unknown;.
  • Default output from the tool suffers from the same problem raised above in the "Errors" section where alternate rule names don't get their own JSDoc comments. I suspect the same fix I describe above would address it.

Overall, because json-schema-to-typescript has about 10x the daily downloads according to npm and uses the same license as this project (MIT), I'm inclined to go with that. I'm going to rebase my condef branch onto the latest next branch now, review the changes tomorrow, and probably commit these changes since it's what I described in the original issue comments. Your good work on defineConfig would still be applicable and could be put into its own PR.

PS - quicktype --src-lang schema ./schema/markdownlint-config-schema.json —out ./lib/configuration.d.ts --just-types --top-level Configuration

DavidAnson added a commit that referenced this pull request Nov 6, 2023
DavidAnson added a commit that referenced this pull request Nov 9, 2023
@DavidAnson
Copy link
Owner

The vite documentation linked above (https://vitejs.dev/config/#config-intellisense) shows a simpler alternative to defineConfig that avoids creating a project dependency and making a function call by relying entirely on the type system. That's clean, efficient, and a generally applicable technique, so I'm going to close this PR as the intended functionality should be available in the next release.. Thank you very much for spending time discussing and working on this!

@DavidAnson DavidAnson closed this Nov 11, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants