-
Notifications
You must be signed in to change notification settings - Fork 770
refactor: Separate out Typescript classes that are shared between Ang… #512
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
…ular & Lit Renderer from the `@a2ui/lit` package into `@a2ui/core`. This change will allow the `@a2ui/angular` to depend on `@a2ui/core` instead of the `@a2ui/lit` package and reduce unnecessary code dependencies. This reduced scope of dependency will also reduce the risk of component tags collision where catalog components defined in Angular space conflicting the catalog components defined in Lit space.
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.
Code Review
This pull request successfully refactors shared TypeScript classes into a new @a2ui/core package, reducing unnecessary code dependencies between @a2ui/angular and @a2ui/lit. The changes involve updating package configurations, import paths, and build scripts across the affected packages. Overall, the refactoring aligns with the goal of modularity and cleaner dependency management. I've identified a few areas related to package entry points, export clarity, and TypeScript configuration that could be improved for better maintainability and consumer experience.
| "main": "./dist/index.js", | ||
| "types": "./dist/index.d.ts", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./dist/src/v0_8/index.d.ts", | ||
| "default": "./dist/src/v0_8/index.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 main and types fields specify ./dist/index.js and ./dist/index.d.ts as the primary entry points, respectively. However, the exports field for the root (.) points to ./dist/src/v0_8/index.js and ./dist/src/v0_8/index.d.ts. This inconsistency can lead to different module resolution behaviors depending on the consumer's environment (e.g., Node.js versions, bundlers). It's best to align these entry points for clarity and predictability, making main and types point to the same location as exports['.'].
| "main": "./dist/index.js", | |
| "types": "./dist/index.d.ts", | |
| "exports": { | |
| ".": { | |
| "types": "./dist/src/v0_8/index.d.ts", | |
| "default": "./dist/src/v0_8/index.js" | |
| "main": "./dist/src/v0_8/index.js", | |
| "types": "./dist/src/v0_8/index.d.ts", | |
| "exports": { | |
| ".": { | |
| "types": "./dist/src/v0_8/index.d.ts", | |
| "default": "./dist/src/v0_8/index.js" | |
| } | |
| }, |
| createSignalA2uiMessageProcessor, | ||
| A2uiMessageProcessor, | ||
| Guards, | ||
| Guards: Core, |
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 Data.Guards property is incorrectly set to re-export the entire @a2ui/core package (Guards: Core,). The original intent was to expose only the guards module from the previous local path. This change significantly broadens the API surface of Data.Guards within the @a2ui/lit package, potentially leading to unexpected dependencies or naming conflicts for consumers. It should specifically re-export the Guards sub-module from @a2ui/core.
| Guards: Core, | |
| Guards: Core.Guards, |
| export * as Primitives from "./types/primitives.js"; | ||
| export * from "./types/primitives.js"; | ||
| export * as Types from "./types/types.js"; | ||
| export * from "./types/colors.js"; | ||
| export * as Styles from "./styles/index.js"; | ||
| export * from "./styles/index.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.
This file contains redundant export * as Name from "./module.js"; statements immediately followed by export * from "./module.js"; for Primitives, Types, and Styles. The export * from syntax already re-exports all named exports from the target module, making the export * as Name from redundant if the intention is just to expose them at the top level. If a specific namespacing is desired, only export * as Name from should be used, otherwise, export * from is sufficient and clearer.
export * from "./types/primitives.js";
export * from "./types/types.js";
export * from "./styles/index.js";| "noUnusedLocals": false, | ||
| "noUnusedParameters": true, |
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 tsconfig.json sets noUnusedLocals to false while noUnusedParameters is true. Allowing unused local variables (noUnusedLocals: false) can hide potential bugs or dead code, impacting maintainability. It's generally a good practice to enable both noUnusedLocals and noUnusedParameters to ensure code cleanliness and catch unused code, or at least be consistent in allowing/disallowing them.
"noUnusedLocals": true,
"noUnusedParameters": true
DO NOT MERGE: This PR introduces a new package
@a2ui/corewhich may need to be released before introducing dependencies to it from@a2ui/angularand@a2ui/lit.Description
Separate out Typescript classes that are shared between Angular & Lit Renderer from the
@a2ui/litpackage into@a2ui/core.This change will allow the
@a2ui/angularto depend on@a2ui/coreinstead of the@a2ui/litpackage and reduce unnecessary code dependencies. This reduced scope of dependency will also reduce the risk of component tags collision where catalog components defined in Angular space conflicting the catalog components defined in Lit space.Tested sample applications
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.