Skip to content
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

externalize all fo pacakges #4970

Merged
merged 1 commit into from
Oct 23, 2024
Merged

externalize all fo pacakges #4970

merged 1 commit into from
Oct 23, 2024

Conversation

sashankaryal
Copy link
Contributor

@sashankaryal sashankaryal commented Oct 23, 2024

This PR adds to the list of externalized FO packages. This is relevant in JS plugin build.

Summary by CodeRabbit

  • New Features
    • Enhanced global scope by adding new properties to the Window interface, providing access to functionalities from the @fiftyone library.
    • Introduced multiple new modules for improved data handling and visualization capabilities.

Copy link
Contributor

coderabbitai bot commented Oct 23, 2024

Walkthrough

The changes in this pull request involve adding multiple import statements for modules from the @fiftyone library in the externalize.ts file. Additionally, new properties are introduced to the global Window interface to expose these modules. Each new property corresponds to a specific module import, ensuring that they are accessible globally when the window object is defined.

Changes

File Path Change Summary
app/packages/plugins/src/externalize.ts Added multiple import statements for modules from @fiftyone and updated the Window interface with new properties.

Possibly related PRs

  • externalize @fiftyone/plugins #4897: This PR modifies the global Window interface by adding a new property __fop__, similar to how the main PR adds multiple new properties to the Window interface for various modules from the @fiftyone library.

Suggested reviewers

  • ritch

Poem

In the code where rabbits play,
New modules hop in, bright and gay.
With properties added to the window wide,
FiftyOne's magic, we now can ride!
So let us dance and celebrate,
For global scope is now first-rate! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sashankaryal sashankaryal changed the base branch from develop to release/v1.1.0 October 23, 2024 14:43
@sashankaryal sashankaryal self-assigned this Oct 23, 2024
Copy link
Contributor

@imanjra imanjra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@sashankaryal sashankaryal merged commit 4f9e9be into release/v1.1.0 Oct 23, 2024
8 of 10 checks passed
@sashankaryal sashankaryal deleted the fix/externalize branch October 23, 2024 14:47
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (1)
app/packages/plugins/src/externalize.ts (1)

Line range hint 1-68: Consider a more scalable approach to package externalization

As the number of externalized packages grows, maintaining individual global assignments might become cumbersome. Consider implementing a more maintainable approach:

  1. Use a single global namespace object (e.g., __fiftyone__)
  2. Implement a package registry pattern
  3. Add package versioning support

This would make it easier to manage package dependencies and avoid global namespace pollution.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 866c8c6 and 32f0a8a.

📒 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 (1)
app/packages/plugins/src/externalize.ts (1)

1-9: LGTM! Import statements follow consistent patterns

The new imports maintain the established naming conventions and import patterns used throughout the codebase.

Comment on lines +60 to +68
window.__foa__ = foa;
window.__focore__ = focore;
window.__foe__ = foe;
window.__fol__ = fol;
window.__fom__ = fom;
window.__fopb__ = fopb;
window.__fosl__ = fosl;
window.__fof__ = fof;
window.__fol3d__ = fol3d;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding error handling for package loading

While the assignments are correct, consider wrapping them in a try-catch block to handle potential module loading failures gracefully. This would help plugin developers diagnose issues more easily.

Here's a suggested implementation:

if (typeof window !== "undefined") {
+  try {
     window.__foa__ = foa;
     window.__focore__ = focore;
     window.__foe__ = foe;
     window.__fol__ = fol;
     window.__fom__ = fom;
     window.__fopb__ = fopb;
     window.__fosl__ = fosl;
     window.__fof__ = fof;
     window.__fol3d__ = fol3d;
+  } catch (error) {
+    console.error("Failed to initialize FiftyOne packages:", error);
+    throw error;
+  }
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
window.__foa__ = foa;
window.__focore__ = focore;
window.__foe__ = foe;
window.__fol__ = fol;
window.__fom__ = fom;
window.__fopb__ = fopb;
window.__fosl__ = fosl;
window.__fof__ = fof;
window.__fol3d__ = fol3d;
if (typeof window !== "undefined") {
try {
window.__foa__ = foa;
window.__focore__ = focore;
window.__foe__ = foe;
window.__fol__ = fol;
window.__fom__ = fom;
window.__fopb__ = fopb;
window.__fosl__ = fosl;
window.__fof__ = fof;
window.__fol3d__ = fol3d;
} catch (error) {
console.error("Failed to initialize FiftyOne packages:", error);
throw error;
}
}

Comment on lines +33 to +41
__foa__: typeof foa;
__focore__: typeof focore;
__foe__: typeof foe;
__fol__: typeof fol;
__fom__: typeof fom;
__fopb__: typeof fopb;
__fosl__: typeof fosl;
__fof__: typeof fof;
__fol3d__: typeof fol3d;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the gathered information, I can now provide the final response:

Documentation needed for window globals in plugin system

The __fo*__ naming pattern is part of the plugin system's implementation but lacks documentation in the codebase. While there's a plugins package with basic documentation, it doesn't explain the window globals pattern. Specific areas needing documentation:

  • The purpose of the __fo*__ naming convention in externalize.ts
  • How these window globals are used in the plugin system
  • Why this approach was chosen for plugin development

This documentation would be valuable since these globals are a core part of the plugin architecture, as evidenced by their extensive use in externalize.ts and the plugin registry in index.ts.

🔗 Analysis chain

Consider documenting the global namespace pattern

While the implementation is correct, consider adding a comment explaining the __fo*__ naming convention and its purpose for plugin development. This would help future maintainers understand the reasoning behind global namespace usage.

Let's verify if the naming pattern is consistently documented:


Let me search more specifically for any documentation related to these global window properties.


Let me check if there's any documentation in the plugins package or related README files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any existing documentation about the __fo*__ pattern
rg -i "naming|pattern|convention.*__fo" --type md

Length of output: 663


Script:

#!/bin/bash
# Search for documentation about window properties or globals
rg -i "(window\.|global).*__fo" -A 2 -B 2

# Also check TypeScript files for any comments explaining this pattern
rg -i "(window\.|global).*__fo.*\n.*\/\/" -A 2 -B 2 --type ts

Length of output: 1981


Script:

#!/bin/bash
# Check for documentation in plugins package
cat app/packages/plugins/README.md 2>/dev/null || echo "No README found"

# Search for any documentation about plugins or window globals
rg -i "plugin|window global" --type md -A 3 -B 3

Length of output: 2853

findtopher pushed a commit that referenced this pull request Oct 23, 2024
findtopher added a commit that referenced this pull request Oct 23, 2024
Co-authored-by: Sashank Aryal <66688606+sashankaryal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants