-
Notifications
You must be signed in to change notification settings - Fork 47
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
refactor(web): clean up deprecated plugin api v1 #1286
Conversation
WalkthroughThis pull request involves the removal of deprecated features from the Re:Earth visualizer plugin API. Specifically, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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
🧹 Outside diff range and nitpick comments (2)
web/src/beta/features/Visualizer/Crust/Plugins/hooks/index.ts (1)
Line range hint
341-350
: Consider documenting the global API exposureThe exposure of the plugin API through
window.reearth
is a critical integration point. Consider adding a code comment explaining the purpose and implications of this global exposure for developers.// expose plugin API for developers + // This global exposure allows external plugins to interact with the Re:Earth visualizer + // Note: The API is cleaned up when the component unmounts useEffect(() => { window.reearth = value.reearth; return () => { window.reearth = undefined; }; }, [value]);web/src/beta/features/Visualizer/Crust/Plugins/pluginAPI/commonReearth.ts (1)
13-17
: Consider namespacing the globalreearth
property to prevent potential conflictsAdding
reearth
to the globalwindow
object can lead to potential naming conflicts or unintended side effects, especially if other scripts define the same global property. To mitigate this risk, consider namespacing or encapsulating the property within a unique object or module to avoid polluting the global namespace.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
web/src/beta/features/Visualizer/Crust/Plugins/hooks/index.ts
(1 hunks)web/src/beta/features/Visualizer/Crust/Plugins/hooks/useDeprecated.ts
(0 hunks)web/src/beta/features/Visualizer/Crust/Plugins/pluginAPI/commonReearth.ts
(7 hunks)web/src/beta/features/Visualizer/Crust/Plugins/pluginAPI/pluginAPIv1Types.ts
(0 hunks)
💤 Files with no reviewable changes (2)
- web/src/beta/features/Visualizer/Crust/Plugins/hooks/useDeprecated.ts
- web/src/beta/features/Visualizer/Crust/Plugins/pluginAPI/pluginAPIv1Types.ts
🔇 Additional comments (4)
web/src/beta/features/Visualizer/Crust/Plugins/hooks/index.ts (2)
234-234
: LGTM: Extension list functionality properly integrated
The addition of getExtensionList
to the reearth API object is consistent with the dependency array updates and properly integrates with the extension management system.
Line range hint 1-350
: Verify the removal of deprecated API usage
The deprecated features have been cleanly removed. Let's verify there are no remaining references to these deprecated APIs in the codebase.
✅ Verification successful
✅ Deprecated API features have been successfully removed
The verification shows that all deprecated plugin API features have been cleanly removed from the codebase. The only remaining references to "deprecated" are:
- In test setup file (
web/src/test/setup.ts
) related to browser API mocking - In changelog entries documenting the removal of deprecated features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to deprecated API features
# Check for any remaining references to useDeprecated
echo "Checking for useDeprecated references..."
rg -l "useDeprecated|deprecated" --type ts --type tsx
# Check for any remaining v1 plugin API types
echo "Checking for v1 plugin API type references..."
rg -l "pluginAPIv1" --type ts --type tsx
# Check for specific deprecated properties that were removed
echo "Checking for specific deprecated properties..."
rg -l "cameraDeprecated|layersDeprecated|sketchDeprecated" --type ts --type tsx
Length of output: 557
Script:
#!/bin/bash
# Description: Search for any remaining references to deprecated API features with correct file type flags
# Check for any remaining references to useDeprecated
echo "Checking for useDeprecated references..."
rg -l "useDeprecated|deprecated"
# Check for any remaining v1 plugin API types
echo "Checking for v1 plugin API type references..."
rg -l "pluginAPIv1"
# Check for specific deprecated properties that were removed
echo "Checking for specific deprecated properties..."
rg -l "cameraDeprecated|layersDeprecated|sketchDeprecated"
# Additional check for any remaining deprecated imports or usages
echo "Checking for deprecated imports and usages..."
rg "import.*deprecated|from.*deprecated"
Length of output: 648
Script:
#!/bin/bash
# Let's check the actual content of files that contain "deprecated" to ensure they're not related to the plugin API
echo "Checking content of web/src/test/setup.ts..."
cat web/src/test/setup.ts
echo "Checking CHANGELOG.md entries for deprecated API removal..."
rg "deprecated" CHANGELOG.md -C 2
echo "Checking for any remaining plugin-related deprecated features..."
rg -g "web/src/beta/features/Visualizer/**" "deprecated" -i
Length of output: 2059
web/src/beta/features/Visualizer/Crust/Plugins/pluginAPI/commonReearth.ts (2)
94-94
: Inclusion of getExtensionList
parameter aligns with updated API
The addition of getExtensionList
to the function parameters ensures that the extension.list
property is available, consistent with the updated CommonReearth
type.
Line range hint 172-343
: Refactoring of commonReearth
return structure effectively removes deprecated properties
The refactored commonReearth
function's return structure removes deprecated properties and simplifies the API, enhancing clarity and maintainability. The current properties and methods are properly exposed, aligning with the updated CommonReearth
type.
Overview
Since we decide to make Visualizer more as an independent product, there's no need to keep the deprecated plugin apis, clean up them also help to make the auto complete more clear.
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
CommonReearth
type, improving the structure of the API.Bug Fixes
Chores