fix: update readonly props without strict mode #679
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Storybook
participant ImageryLayer
User->>Storybook: Select tile ("arcgis" or "cesium")
Storybook->>ImageryLayer: Render with selected imageryProvider
ImageryLayer->>ImageryLayer: Update imageryProvider
ImageryLayer-->>User: Display selected imagery
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/ImageryLayer/ImageLayer.stories.tsx (3)
17-30: LGTM: Enhanced story interactivity with tile selection.The changes to the Basic story improve its functionality by allowing users to select between different imagery providers. The conditional logic in the render function correctly implements this feature.
Consider extracting the URL for the ArcGIS imagery provider into a constant at the top of the file for better maintainability:
const ARCGIS_WORLD_STREET_MAP_URL = "https://services.arcgisonline.com/ArcGIS/rest/services/World_Street_Map/MapServer";Then use this constant in the render function:
ArcGisMapServerImageryProvider.fromUrl(ARCGIS_WORLD_STREET_MAP_URL)
37-58: LGTM: Added Strict story for StrictMode testing.The addition of the Strict story is a good practice, allowing the component to be tested under React's StrictMode. The implementation correctly replicates the functionality of the Basic story within a StrictMode wrapper.
To reduce code duplication between the Basic and Strict stories, consider extracting the common logic into a separate function:
const createStoryRender = (Wrapper = React.Fragment) => ({ tile, ...args }: any) => ( <Wrapper> <Viewer full> <ImageryLayer {...args} imageryProvider={ tile === "arcgis" ? ArcGisMapServerImageryProvider.fromUrl(ARCGIS_WORLD_STREET_MAP_URL) : IonImageryProvider.fromAssetId(IonWorldImageryStyle.AERIAL) } /> <ImageryLayer alpha={0.5} imageryProvider={IonImageryProvider.fromAssetId(3812, {})} /> </Viewer> </Wrapper> ); export const Basic: Story = { argTypes: { tile: { options: ["arcgis", "cesium"], control: { type: "select" } }, } as any, render: createStoryRender(), }; export const Strict: Story = { argTypes: { tile: { options: ["arcgis", "cesium"], control: { type: "select" } }, } as any, render: createStoryRender(StrictMode), };This approach would make the code more DRY and easier to maintain.
Line range hint
1-58: Overall, great improvements to the ImageryLayer stories!The changes in this file enhance both the functionality and testability of the ImageryLayer component in Storybook. The addition of tile selection options and the new Strict story provide more comprehensive testing scenarios. These modifications align well with the PR objectives and improve the overall quality of the component's stories.
Consider implementing a custom hook or utility function for creating imagery providers based on the tile selection. This could be reused across different components and stories, promoting code reuse and maintainability:
const useImageryProvider = (tile: string) => { return useMemo(() => { if (tile === "arcgis") { return ArcGisMapServerImageryProvider.fromUrl(ARCGIS_WORLD_STREET_MAP_URL); } return IonImageryProvider.fromAssetId(IonWorldImageryStyle.AERIAL); }, [tile]); };This approach would centralize the logic for creating imagery providers and make it easier to add new provider types in the future.
src/ImageryLayer/ImageryLayer.ts (1)
111-112: Approved: Improved waiting list managementThe new implementation for removing awaited imagery providers from the waiting list is more efficient and clearer than the previous filter method. It directly modifies the array using
splice, which is a good practice.However, we can further optimize this code by combining the
indexOfandspliceoperations:context.__$internal.imageryLayerWaitingList.splice( context.__$internal.imageryLayerWaitingList.indexOf(imageryProvider), 1 );This change would slightly improve performance by avoiding the creation of an intermediate variable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/ImageryLayer/ImageLayer.stories.tsx (2 hunks)
- src/ImageryLayer/ImageryLayer.ts (2 hunks)
- src/core/hooks.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/ImageryLayer/ImageLayer.stories.tsx (1)
2-2: LGTM: Import statement updated correctly.The addition of
IonWorldImageryStyleimport from thecesiumlibrary is consistent with its usage in the updated render functions for both stories.src/ImageryLayer/ImageryLayer.ts (2)
58-58: LGTM: Improved type definition forImageryLayerPropsThe updated type definition for
ImageryLayerPropscorrectly combinesImageryLayerCesiumPropsandImageryLayerCesiumReadonlyProps. This change enhances type safety and allows the component to handle both synchronous and asynchronous imagery providers, which aligns with the PR objectives.
Line range hint
1-138: Overall assessment: Improvements to ImageryLayer componentThe changes in this file successfully address the PR objectives by updating the
ImageryLayercomponent to handle both synchronous and asynchronous imagery providers. The type definitions have been improved, and the waiting list management has been optimized. These changes enhance the component's flexibility and performance.To ensure the changes are working as expected, consider adding or updating unit tests to cover the new functionality, particularly the handling of asynchronous imagery providers and the updated waiting list management logic.
To verify the changes, run the following script:
src/core/hooks.ts (1)
290-291: Approve: Improved lifecycle managementThis change enhances the robustness of the
useCesiumComponenthook by ensuring that any ongoing unmount process is completed before updating properties. It prevents potential race conditions between unmounting and updating, which could lead to errors or unexpected behavior.The addition of:
if (unmountReadyRef.current) { await unmountReadyRef.current; }is a necessary improvement that aligns with the existing pattern of managing asynchronous operations in the component lifecycle.
Summary by CodeRabbit
New Features
ImageryLayercomponent with selectable imagery providers ("arcgis" and "cesium").Strict, for additional rendering options.Bug Fixes
ImageryLayercomponent.Refactor