Conversation
…ditControl component to TypeScript. Update webpack configuration to handle TypeScript files and remove deprecated JavaScript files. Add example usage of EditControl in TypeScript. Clean up unused files and ensure proper type definitions for the EditControl component.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the react-leaflet-draw-next library from JavaScript to TypeScript. The migration converts the main component to TypeScript, adds comprehensive type definitions, and updates the build configuration to support TypeScript compilation.
- Replaces JavaScript implementation with TypeScript equivalents using proper type annotations
- Adds comprehensive type definitions for Leaflet draw events and control options
- Updates build toolchain to use TypeScript compiler and ts-loader instead of Babel
Reviewed Changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.config.js | Adds TypeScript file extensions and ts-loader configuration |
| src/types.ts | Defines comprehensive TypeScript interfaces for draw events and control options |
| src/index.ts | Updates entry point to export TypeScript types |
| src/EditControl.tsx | Complete TypeScript rewrite of the main component with proper type annotations |
| src/EditControl.js | Removes the original JavaScript implementation |
| package.json | Updates build scripts and types path for TypeScript compilation |
| examples/class/webpack.config.js | Configures example webpack for TypeScript support |
| examples/class/tsconfig.json | Adds TypeScript configuration for examples |
| examples/class/index.tsx | Converts example entry point to TypeScript |
| examples/class/edit-control.tsx | Converts example component to TypeScript with proper typing |
| examples/class/edit-control.js | Removes original JavaScript example |
| if (featureGroup) { | ||
| featureGroup.addLayer(e.layer); | ||
| } | ||
| onCreated && (onCreated as unknown as (e: L.LeafletEvent) => void)(e); |
There was a problem hiding this comment.
The type assertion (onCreated as unknown as (e: L.LeafletEvent) => void) is unnecessarily complex and unsafe. Since onCreated is already properly typed in the interface, you can call it directly: onCreated && onCreated(e);
| onCreated && (onCreated as unknown as (e: L.LeafletEvent) => void)(e); | |
| onCreated && onCreated(e); |
| const options: L.Control.DrawConstructorOptions = { | ||
| edit: { | ||
| ...(edit ?? {}), | ||
| featureGroup: featureGroup as unknown as L.FeatureGroup, |
There was a problem hiding this comment.
The type assertion featureGroup as unknown as L.FeatureGroup is unsafe and could hide type mismatches. Consider using a more specific type guard or adjusting the generic type constraint to ensure type safety.
| featureGroup: featureGroup as unknown as L.FeatureGroup, | |
| featureGroup: featureGroup as L.FeatureGroup<L.Layer>, |
|
|
||
| if (draw) { | ||
| // Cast is safe: keys align with L.Control.DrawOptions | ||
| options.draw = { ...(draw as L.Control.DrawOptions) }; |
There was a problem hiding this comment.
The type assertion draw as L.Control.DrawOptions assumes the types are compatible without verification. Consider using a proper type guard or ensuring the DrawOptions interface properly extends L.Control.DrawOptions to avoid potential runtime issues.
| options.draw = { ...(draw as L.Control.DrawOptions) }; | |
| if (isDrawOptions(draw)) { | |
| options.draw = { ...draw }; | |
| } else { | |
| // Optionally, warn or throw if draw is invalid | |
| if (process.env.NODE_ENV !== 'production') { | |
| // eslint-disable-next-line no-console | |
| console.warn('draw prop does not conform to L.Control.DrawOptions:', draw); | |
| } | |
| } |
| }; | ||
|
|
||
| render() { | ||
| const fgInstance = this.featureGroupRef.current as unknown as L.FeatureGroup | undefined; |
There was a problem hiding this comment.
The type assertion as unknown as L.FeatureGroup | undefined is overly broad and unsafe. Consider using a more specific type assertion or checking the actual type at runtime to ensure type safety.
| const fgInstance = this.featureGroupRef.current as unknown as L.FeatureGroup | undefined; | |
| const fgInstance = (this.featureGroupRef.current instanceof L.FeatureGroup) | |
| ? this.featureGroupRef.current | |
| : undefined; |
| attribution='© <a href="http://osm.org/copyright">OpenStreetMap</a> contributors' | ||
| url="http://{s}.tile.osm.org/{z}/{x}/{y}.png" | ||
| /> | ||
| <FeatureGroup ref={this.featureGroupRef as any}> |
There was a problem hiding this comment.
Using as any defeats the purpose of TypeScript type checking. Consider using a properly typed ref or a more specific type assertion to maintain type safety.
| <FeatureGroup ref={this.featureGroupRef as any}> | |
| <FeatureGroup | |
| ref={(ref) => { | |
| // Assign both the class ref and _editableFG for compatibility | |
| (this.featureGroupRef as React.MutableRefObject<L.FeatureGroup | null>).current = ref; | |
| this._editableFG = ref; | |
| }} | |
| > |
| url="http://{s}.tile.osm.org/{z}/{x}/{y}.png" | ||
| /> | ||
| <FeatureGroup ref={this.featureGroupRef as any}> | ||
| {fgInstance && (fgInstance as any)._leaflet_id && ( |
There was a problem hiding this comment.
Using (fgInstance as any)._leaflet_id to access private Leaflet properties is fragile and defeats TypeScript's type safety. Consider using a proper type guard or checking if the instance has the expected properties through a safer approach.
| {fgInstance && (fgInstance as any)._leaflet_id && ( | |
| {fgInstance && '_leaflet_id' in fgInstance && ( |
| _onChange = () => { | ||
| const { onChange } = this.props; | ||
| if (!this._editableFG || !onChange) return; | ||
| const geojsonData = (this._editableFG as any).toGeoJSON(); |
There was a problem hiding this comment.
Using as any to access the toGeoJSON() method bypasses TypeScript's type checking. Consider properly typing _editableFG or using a type assertion that maintains some level of type safety.
| const geojsonData = (this._editableFG as any).toGeoJSON(); | |
| const geojsonData = (this._editableFG as L.FeatureGroup).toGeoJSON(); |
No description provided.