-
Notifications
You must be signed in to change notification settings - Fork 15
chore: add zod2md-nx-plugin #1162
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
base: main
Are you sure you want to change the base?
Conversation
|
View your CI Pipeline Execution ↗ for commit 8db0605
☁️ Nx Cloud last updated this comment at |
@code-pushup/ci
@code-pushup/cli
@code-pushup/core
@code-pushup/create-cli
@code-pushup/models
@code-pushup/nx-plugin
@code-pushup/axe-plugin
@code-pushup/coverage-plugin
@code-pushup/eslint-plugin
@code-pushup/js-packages-plugin
@code-pushup/jsdocs-plugin
@code-pushup/lighthouse-plugin
@code-pushup/typescript-plugin
@code-pushup/utils
commit: |
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit b2e8c67 with previous commit f74d2dd. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories👎 8 groups regressed, 👍 3 audits improved, 👎 13 audits regressed, 27 audits changed without impacting score🗃️ Groups
13 other groups are unchanged. 🛡️ Audits
635 other audits are unchanged. |
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
This PR moves the transformer logic into tools and makes it configurable Followup PR: #1162 --------- Co-authored-by: John Doe <john.doe@example.com> Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
| "vscode-material-icons": "^0.1.0", | ||
| "zod": "^4.0.5" | ||
| "zod": "^4.0.5", | ||
| "zod2md": "^0.2.4" |
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.
No, this is wrong. Think about what you are doing, please.
The package's dependencies should only list production dependencies, i.e. the minimum the end-user needs to have installed. Development tools like vitest or zod2md do not belong here.
The @nx/dependency-checks lint rule determines what the production dependencies should be by checking imports in all files matched by the production cache input. This is why we had the "!{projectRoot}/zod2md.config.ts" pattern in nx.json, because we know that zod2md.config.ts files aren't part of production builds. I've no idea why you removed that line.
| "!{projectRoot}/CHANGELOG.md", | ||
| "!{projectRoot}/perf/**/*", | ||
| "!{projectRoot}/tools/**/*", | ||
| "!{projectRoot}/zod2md.config.ts", |
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.
Please restore this line - see previous comment.
| "generate-docs", | ||
| "ts-patch", |
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.
Why doesn't the plugin add these dependencies? The whole idea is that each project creates a config file (zod2md.config.ts), and the Nx plugin (zod2md-jsdocs) takes care of everything else.
Also, I wouldn't place the ts-patch here. The previous setup was simpler, as there was a single tspatch (actually called pre-build before) target in zod2md-jsdocs, and it was a dependency of zod2md-jsdocs's build target. This way, there's only 1 target in 1 project. Which makes sense, because installing ts-patch isn't something that needs to run per project, our development dependencies are installed globally.
| /** | ||
| * Creates the ts-patch target configuration | ||
| * @returns {object} ts-patch target configuration | ||
| */ | ||
| const createTsPatchTargetConfig = { | ||
| command: 'ts-patch install', | ||
| cache: true, | ||
| inputs: ['sharedGlobals', { runtime: 'ts-patch check' }], | ||
| }; |
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.
I would move this target into tools/zod2md-jsdocs/project.json - see previous comment.
| "pre-build": { | ||
| "command": "ts-patch install", | ||
| "cache": true, | ||
| "inputs": ["sharedGlobals", { "runtime": "ts-patch check" }] | ||
| } |
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.
I would restore this target - see previous comment. I'm fine with renaming pre-build to tspatch, though.
tools/zod2md-jsdocs/eslint.config.js
Outdated
| { | ||
| files: ['**/*.ts', '**/*.cjs', '**/*.js'], | ||
| rules: { | ||
| 'import/no-commonjs': 'off', | ||
| '@typescript-eslint/no-require-imports': 'off', | ||
| '@typescript-eslint/no-unused-vars': 'off', | ||
| 'unicorn/prefer-module': 'off', | ||
| 'functional/immutable-data': 'off', | ||
| 'arrow-body-style': 'off', | ||
| }, | ||
| }, |
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.
You don't need to disable any of these rules. Some of them don't actually have any errors (no-unused-vars and arrow-body-style). All the others are caused by using require('...') and module.exports = ... in .ts files.
I understand that Nx plugins have to run in CJS. But if you're using .ts, you can use normal import/export syntax anyway, because it gets transpiled to CJS syntax in .js files.
| { | |
| files: ['**/*.ts', '**/*.cjs', '**/*.js'], | |
| rules: { | |
| 'import/no-commonjs': 'off', | |
| '@typescript-eslint/no-require-imports': 'off', | |
| '@typescript-eslint/no-unused-vars': 'off', | |
| 'unicorn/prefer-module': 'off', | |
| 'functional/immutable-data': 'off', | |
| 'arrow-body-style': 'off', | |
| }, | |
| }, |
Our regular lint config will pass if you convert the .ts files to use ESM imports/exports.
diff --git a/tools/zod2md-jsdocs/src/index.ts b/tools/zod2md-jsdocs/src/index.ts
index 461bc243a..a44fd9901 100644
--- a/tools/zod2md-jsdocs/src/index.ts
+++ b/tools/zod2md-jsdocs/src/index.ts
@@ -1,4 +1,3 @@
-const transformers = require('./lib/transformers.js');
+import transformers from './lib/transformers.js';
-module.exports = transformers;
-module.exports.default = transformers;
+export default transformers;
diff --git a/tools/zod2md-jsdocs/src/lib/transformers.ts b/tools/zod2md-jsdocs/src/lib/transformers.ts
index dc146fd1d..4b326d6e3 100644
--- a/tools/zod2md-jsdocs/src/lib/transformers.ts
+++ b/tools/zod2md-jsdocs/src/lib/transformers.ts
@@ -1,7 +1,5 @@
import type { PluginConfig, TransformerExtras } from 'ts-patch';
-import type * as ts from 'typescript';
-
-const tsInstance: typeof ts = require('typescript');
+import ts from 'typescript';
/**
* Generates a JSDoc comment for a given type name and base URL.
@@ -36,7 +34,7 @@ function annotateTypeDefinitions(
'Please configure it in your tsconfig.json plugins section.',
);
}
- const tsLib = extras?.ts ?? tsInstance;
+ const tsLib = extras?.ts ?? ts;
return (context: ts.TransformationContext) => {
const visitor = (node: ts.Node): ts.Node => {
if (
@@ -59,4 +57,4 @@ function annotateTypeDefinitions(
};
}
-module.exports = annotateTypeDefinitions;
+export default annotateTypeDefinitions;
Note that the nx-plugin.cjs file also doesn't violate our lint config, despite using require and module.exports. This is because our config is smart enough to recognize that it makes no sense to enforce ESM syntax in .cjs files:
| // nx.json | ||
| { | ||
| //... | ||
| "plugins": ["./tools/zod2md-jsdocs-nx-plugin/src/lib/plugin.js"], |
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.
The path ./tools/zod2md-jsdocs-nx-plugin/src/lib/plugin.js is incorrect; it's actually ./tools/zod2md-jsdocs/dist/src (as used in nx.json). It appears several times in this document.
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Followup of #1163
This PR includes:
zod2md-jsdocszod2md.config.tsA potential followup PR could implement a nx plugin for our
buildtarget and adddependsOnsettings automatically. This is also noted in a clean up issue.Note
This is tooling code and it is not planned to get released
Related:
#1168