-
-
Notifications
You must be signed in to change notification settings - Fork 254
Implement proper js module system for bit Boilerplate (#11542) #11543
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
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRestructures the TypeScript module system from single-file bundled output to an ES module-based architecture. Centralizes type declarations in a dedicated typings.d.ts file, exports classes as modules, introduces index.ts as the entry point for coordinating initialization and global window attachment, and updates the build pipeline with esbuild bundling. Changes
Sequence DiagramsequenceDiagram
actor Build
participant tsc as TypeScript Compiler
participant esbuild as esbuild Bundler
participant index as Scripts/index.ts
participant app as App Module
participant webinterop as WebInteropApp Module
participant ads as Ads Module (conditional)
participant window as Global Window
Build->>tsc: Compile TypeScript files
tsc->>tsc: Output ES modules
Build->>esbuild: Bundle index.js
esbuild->>index: Load entry point
index->>app: import App
index->>webinterop: import WebInteropApp
alt Ads enabled
index->>ads: import Ads
end
index->>window: app.App = App
index->>window: window.WebInteropApp = WebInteropApp
alt Ads enabled
index->>window: window.Ads = Ads
end
esbuild->>esbuild: Output app.js bundle
esbuild-->>Build: Complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Pull Request Overview
This PR migrates the TypeScript build system from a single-file compilation approach to a module-based architecture with bundling. The changes enable proper ES module syntax with imports/exports while maintaining the same runtime behavior through esbuild bundling.
- TypeScript files now use ES module syntax with import/export statements
- Shared type definitions moved to a central typings.d.ts file
- Build process updated to compile TypeScript to individual modules, then bundle with esbuild
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Updated to use ES2015 modules with node resolution instead of outFile compilation |
| typings.d.ts | New file centralizing type definitions for Blazor, BitButil, and BitTheme |
| theme.ts | Removed inline type declarations now defined in typings.d.ts |
| index.ts | New entry point that imports all modules and exposes classes to window global |
| events.ts | Added import for App class |
| bswup.ts | Added import for App class |
| WebInteropApp.ts | Changed to export class, removed inline BitButil types and window assignment |
| App.ts | Changed to export class, removed inline DotNetObject type and window assignment |
| Ads.ts | Changed to export class, removed window assignment |
| Boilerplate.Client.Core.csproj | Added esbuild bundling step to create final app.js from compiled modules |
| .gitignore | Added pattern to ignore intermediate .js files in Scripts folder |
...oilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Boilerplate.Client.Core.csproj
Show resolved
Hide resolved
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/typings.d.ts (1)
11-12: Consider more specific types for WebAuthn methods.The
BitButil.webAuthnmethods currently useunknownfor both parameters and return types. While this is safe, it limits type checking and IDE assistance. If the WebAuthn API shapes are stable, consider defining specific interfaces for credential options and responses.Example refactor:
// Bit.Butil types: interface WebAuthnCredentialOptions { challenge: BufferSource; // ... other known properties } interface WebAuthnCredential { id: string; type: string; // ... other known properties } declare class BitButil { static webAuthn: { getCredential: (options: WebAuthnCredentialOptions) => Promise<WebAuthnCredential>, createCredential: (options: WebAuthnCredentialOptions) => Promise<WebAuthnCredential> } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (11)
src/Templates/Boilerplate/Bit.Boilerplate/.gitignore(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Boilerplate.Client.Core.csproj(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/Ads.ts(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/App.ts(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/WebInteropApp.ts(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/bswup.ts(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/events.ts(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/index.ts(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/theme.ts(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/typings.d.ts(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/tsconfig.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: build and test
🔇 Additional comments (20)
src/Templates/Boilerplate/Bit.Boilerplate/.gitignore (1)
229-229: LGTM! Properly excludes generated intermediate files.The new ignore pattern correctly excludes TypeScript-generated JavaScript files in the Scripts folder, which are intermediate artifacts before esbuild bundles them into the final app.js.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/Ads.ts (1)
4-4: LGTM! Proper module export pattern.The class is now exported as a module, and global window exposure is correctly delegated to the index.ts entry point where it's conditionally exposed based on the ads template directive.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/bswup.ts (1)
3-3: LGTM! Correct module import.The import is necessary since App is no longer globally attached to window. The named import syntax correctly matches the export in App.ts.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/events.ts (1)
3-3: LGTM! Correct module import.The import enables module-based access to App.publishMessage at line 21, replacing the previous reliance on global window attachment.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/tsconfig.json (1)
6-9: LGTM! Critical configuration for ES module support.These changes directly address the PR objectives:
"module": "es2015"enables modern ES module imports/exports between TypeScript files"moduleResolution": "node"allows TypeScript to reliably resolve imports- Removing
outFileshifts bundling responsibility to esbuild in the build pipelineThis is the foundation for the proper module system implementation.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/App.ts (2)
3-3: LGTM! Proper module export.The App class is now correctly exported as a module, with global window exposure delegated to index.ts.
5-7: DotNetObject type is properly declared.The verification confirms that
DotNetObjectis correctly declared intypings.d.tsat line 2 withdeclare interface DotNetObject {. The type is available for use inApp.tsand there are no issues to address.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Boilerplate.Client.Core.csproj (2)
68-72: LGTM! Proper build pipeline for module bundling.The build sequence is correct:
- TypeScript compilation (tsc) generates ES modules in Scripts/
- esbuild bundles Scripts/index.js → wwwroot/scripts/app.js
- Optional minification for Production builds
This properly implements the module-based architecture described in the PR objectives.
70-70: esbuild is properly declared in devDependencies (version 0.25.11).The build step correctly references
node_modules/.bin/esbuild, which will be available after npm install. No changes needed.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/index.ts (3)
3-10: LGTM! Well-structured entry point imports.The import organization is excellent:
- Side-effect imports (lines 3-5) execute initialization code from bswup, theme, and events modules
- Named imports (lines 6-7) bring in classes for global exposure
- Conditional compilation (lines 8-10) properly handles the optional Ads feature
This establishes a clear module initialization order as specified in the PR objectives.
12-17: LGTM! Proper global exposure for Blazor interop.The centralized window exposure pattern:
- Maintains backward compatibility with C# JSInterop calls
- Conditionally exposes Ads only when the feature is enabled
- Provides a single source of truth for global API surface
This addresses the PR objective of defining a clear entry point that coordinates initialization and global exposure.
1-17: Excellent architectural implementation of the entry point module!This file directly addresses the PR objectives by:
- Defining a clear entry point that coordinates imports and initialization
- Establishing explicit module relationships and execution order
- Centralizing global window exposure for Blazor interop
- Supporting conditional feature inclusion via template directives
The pattern of "modules export, entry point exposes" is a clean separation of concerns that will improve maintainability.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/WebInteropApp.ts (3)
1-1: Clarify the//+:cnd:noEmitdirective.The
//+:cnd:noEmitcomment appears to be a build tool directive but lacks documentation. Consider adding a brief inline comment explaining its purpose for maintainability.Can you confirm what build tool processes this directive and document it?
4-4: LGTM: Export statement aligns with ES module objectives.Converting the class to an exported module enables proper import/export usage across TypeScript files, directly addressing the PR objective to replace the single-file outFile approach.
86-86: BitButil type resolution verified—no issues found.The verification confirms:
- BitButil is declared once in typings.d.ts (line 9) as
declare class BitButilwith staticwebAuthnproperty- Both methods (
getCredential,createCredential) are properly typed and match the usage at lines 86 and 102 in WebInteropApp.ts- No redeclarations or shadowing detected in the codebase
- TypeScript compiler will correctly resolve these types globally
The module system changes do not introduce type resolution problems.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/typings.d.ts (2)
1-29: LGTM: Successfully centralizes type declarations per PR objectives.This new file effectively consolidates TypeScript declarations for external libraries (Blazor, Bit.Butil, Bit.BlazorUI), eliminating the scattered inline declarations mentioned in the PR objectives. The structure is clear and maintainable.
19-27: Well-designed BitThemeOptions interface.The interface provides comprehensive configuration options with appropriate optional flags, theme variants, and a typed callback. The design supports both automatic (system/persist) and manual theme management effectively.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Scripts/theme.ts (3)
6-6: LGTM: Optional chaining provides defensive initialization.The optional chaining operator
BitTheme?.initgracefully handles scenarios where the Bit.BlazorUI library may not be loaded, preventing runtime errors. This is good defensive coding practice for modular architecture.
13-14: Good guard against missing CSS variables.The early return when
primaryBgColoris unavailable prevents potential issues with setting an emptytheme-colormeta tag. This handles edge cases where CSS variables might not be initialized during theme changes.
1-19: LGTM: Clean separation of type declarations from runtime logic.Successfully moves type declarations to
typings.d.tswhile preserving the runtime initialization logic. This separation aligns with the PR objective to centralize TypeScript declarations and improves maintainability. The updated comment on line 3 clearly explains the module's purpose.
closes #11542
Summary by CodeRabbit