-
Notifications
You must be signed in to change notification settings - Fork 0
feat(): migration to esm #12
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
Conversation
.cursor/mcp.json
Outdated
| "args": [ | ||
| "./packages/angular-mcp/dist/main.js", | ||
| "--workspaceRoot=/home/spoltorak/projects/x-mcp", | ||
| "--workspaceRoot=/Users/kyrylokarnaukhov/Documents/projects/angular-mcp-toolkit", |
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.
Lets avoid using local paths in the source
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.
Done. Local MCP is not tracked anymore, example file available
docs/architecture-internal-design.md
Outdated
| | **Resource Provider** | `registerResources()` | Extend logic to aggregate custom docs or design-system assets. | | ||
|
|
||
| All tools share the `ToolsConfig` interface (`@push-based/models`) that bundles: | ||
| All tools share the `ToolsConfig` interface (`@code-pushup/models`) that bundles: |
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 not tia everything to CP.. The reason we have out own models and utils is to decouple a bit
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.
All tools are sharing @push-based/models ToolsConfig. Import name fixed.
docs/writing-custom-tools.md
Outdated
| ```ts | ||
| import { z } from 'zod'; | ||
| import { ToolSchemaOptions, ToolsConfig } from '@push-based/models'; | ||
| import { ToolSchemaOptions, ToolsConfig } from '@code-pushup/models'; |
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 definitely should be '@push-based/models';
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.
Yes, fixed
| const module = await import(absPath); | ||
|
|
||
| // Handle both ES modules (export default) and CommonJS (module.exports) | ||
| // Support: export default dsComponents, module.exports = { dsComponents }, or direct module.exports = dsComponents | ||
| const dsComponents = module.default || module.dsComponents || module; | ||
| // Pure ESM: use export default | ||
| const dsComponents = module.default; |
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 seems quickly added instead of thought through
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.
Open for suggestion, only thing that was not breaking tools.
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.
If its needed to make it work, we could think of some reusable thing or docs on the WHY.
The case module.default || module; e.g. ise repeated module.dsComponents not.
It will help to establish a pattern that others can follow more easily.
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.
|
|
||
| // Handle both ES modules (export default) and CommonJS (module.exports) | ||
| const rawData = module.default || module.dsComponents || module; | ||
| // Pure ESM: use export default |
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.
Could we avoid those kind of polluting comments pls
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.
Fixed 1f4a181
|
|
||
| // Handle both ES modules (export default) and CommonJS (module.exports) | ||
| dsComponents = module.default || module.dsComponents || module; | ||
| // Pure ESM: use export default |
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.
remove pls. Same on all other places
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.
Fixed 1f4a181
BioPhoton
left a comment
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.
Added some Comments.
In general the PR was quite big which makes it harder to review and it takes longer. Also the quality of the review drops at a certain volumr of changes.
The first and best thing is to:
- chunck big PR into smaller pieces
- do a self revirew before commiting code or asking for feedback
|
Commits focus description for easier navigation
|
BioPhoton
left a comment
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.
Added comments
.cursor/mcp.json
Outdated
| @@ -1,22 +0,0 @@ | |||
| { | |||
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 do we remove the setup? I think it's quite helpful for testing too.. maybe a postinstall?
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.
We have example file for this that should be copied and renamed to mcp.json by developer who want to use it, having just mcp.json causes override hell from dev to dev.
ESM Migration and Dependency Consolidation
Summary
Migrates the Angular MCP Toolkit from CommonJS to ES Modules (ESM) and consolidates dependencies by adopting
@code-pushup/*packages to replace internal shared libraries.Technical Changes
Module System Migration
"type": "module"to rootpackage.jsonjest.preset.js→jest.preset.mjswith ESM export syntaxwebpack.config.js→webpack.config.cjsto maintain CommonJS compatibilitytsconfig.jsonfiles to remove CommonJS-specific settingsDependency Consolidation
@push-based/*packages to@code-pushup/*equivalents:@push-based/models→@code-pushup/models@push-based/utils→@code-pushup/utils@code-pushup/corepackages/shared/angular-cli-utilspackage (149 lines removed)packages/shared/models(3,000+ lines removed)Import Statement Updates
Build System Changes
package.jsonfiles across all workspace packages to reflect new dependenciestsconfig.lib.jsonfiles for ESM compatibilityFiles Modified
Impact
Breaking Changes
@code-pushup/*packages