externalize @fiftyone/plugins #4897
Conversation
WalkthroughThe changes introduce a new import for the Changes
Possibly related PRs
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
|
sashankaryal
force-pushed
the
fix/plugin-externalization
branch
from
f8384df
to
2210188
Compare
![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 (2)
app/packages/plugins/src/externalize.ts (2)
23-23: LGTM: Window interface updated for FiftyOne pluginsThe global
Windowinterface is correctly updated to include the__fop__property for FiftyOne plugins. This change is consistent with how other FiftyOne modules are handled and supports the PR objective.Consider documenting the reason for using double underscores in property names, as this convention typically indicates internal or private properties. While it's consistent with other properties in this file, it might be worth explaining this choice for future maintainers.
41-41: LGTM: Global assignment for FiftyOne plugins addedThe assignment of the
fopmodule towindow.__fop__is correctly implemented, making the FiftyOne plugins available globally. This change directly supports the PR objective and is consistent with how other modules are handled.Consider grouping related assignments together for better code organization. For instance, you could move this line up to be adjacent to other FiftyOne module assignments (like
__fos__,__foc__, etc.) rather than placing it between__mui__and__styled__.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/packages/plugins/src/externalize.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/plugins/src/externalize.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/plugins/src/externalize.ts (2)
6-6: LGTM: Import statement for FiftyOne plugins addedThe import statement for
@fiftyone/pluginsis correctly added and follows the established naming convention used for other FiftyOne modules. This change aligns with the PR objective of enabling the referencing of FiftyOne plugins.
Line range hint
1-43: Summary: FiftyOne plugins successfully externalizedThe changes in this file successfully implement the externalization of the FiftyOne plugins module. The implementation is consistent with the existing code patterns and directly supports the PR objective. The import, global interface modification, and window assignment are all correctly implemented.
Minor suggestions were made regarding code organization and documentation, but these are not critical issues. Overall, the changes look good and achieve the intended goal of enabling the referencing of FiftyOne plugins by JavaScript panels.
This makes it so that fiftyone plugins can be referenced by JS panels.
Summary by CodeRabbit
@fiftyone/pluginsmodule into the global scope for enhanced accessibility across the application.__fop__to the globalWindowinterface for seamless usage of thefiftyone/pluginsmodule.