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.

Sign up for GitHub

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

Jump to bottom

externalize all fo pacakges #4970

Merged
merged 1 commit into from
Oct 23, 2024
Merged

externalize all fo pacakges #4970

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions app/packages/plugins/src/externalize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
import * as foa from "@fiftyone/aggregations";
import * as focore from "@fiftyone/core";
import * as foe from "@fiftyone/embeddings";
import * as fol from "@fiftyone/looker";
import * as fom from "@fiftyone/map";
import * as fopb from "@fiftyone/playback";
import * as fosl from "@fiftyone/spotlight";
import * as fof from "@fiftyone/flashlight";
import * as fol3d from "@fiftyone/looker-3d";
import * as foc from "@fiftyone/components";
import * as foo from "@fiftyone/operators";
import * as fos from "@fiftyone/state";
Expand All @@ -21,6 +30,15 @@ declare global {
__foo__: typeof foo;
__fosp__: typeof fosp;
__fop__: typeof fop;
__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;
Comment on lines +33 to +41
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

__mui__: typeof mui;
__styled__: typeof styled;
}
Expand All @@ -39,5 +57,14 @@ if (typeof window !== "undefined") {
window.__fosp__ = fosp;
window.__mui__ = mui;
window.__fop__ = fop;
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;
Comment on lines +60 to +68
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;
}
}

window.__styled__ = styled;
}
Loading