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

fix externalize bug #4984

Merged
merged 2 commits into from
Oct 24, 2024
Merged

fix externalize bug #4984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2ae06b
fix externalize bug
sashankaryal Oct 24, 2024
23a6146
undo vite removal
sashankaryal Oct 24, 2024
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
5 changes: 1 addition & 4 deletions app/packages/looker-3d/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"files": [
"dist"
],
"main": "src/Looker3d.tsx",
"main": "./src/index.ts",
"dependencies": {
"@emotion/react": "^11.11.3",
"@emotion/styled": "^11.11.0",
Expand Down Expand Up @@ -45,8 +45,5 @@
"@react-spring/web": "*",
"recoil": "*"
},
"fiftyone": {
"script": "dist/index.umd.js"
},
"packageManager": "yarn@3.2.1"
}
1 change: 1 addition & 0 deletions app/packages/looker-3d/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./Looker3d";
33 changes: 16 additions & 17 deletions app/packages/plugins/src/externalize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
import * as foa from "@fiftyone/aggregations";
import * as focore from "@fiftyone/core";
import * as foe from "@fiftyone/embeddings";
import * as foc from "@fiftyone/components";
import * as fof from "@fiftyone/flashlight";
import * as fol from "@fiftyone/looker";
import * as fom from "@fiftyone/map";
import * as foo from "@fiftyone/operators";
import * as fopb from "@fiftyone/playback";
import * as fop from "@fiftyone/plugins";
import * as fosp from "@fiftyone/spaces";
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";
import * as fou from "@fiftyone/utilities";
import * as fosp from "@fiftyone/spaces";
import * as fop from "@fiftyone/plugins";
import * as mui from "@mui/material";
import React from "react";
import ReactDOM from "react-dom";
Expand All @@ -31,16 +27,18 @@ declare global {
__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;
__mui__: typeof mui;
__styled__: typeof styled;

// todo: the following cannot be externalized because of unknown reason
// __focore__: typeof focore;
// __fol3d__: typeof fol3d;
// __fom__: typeof fom;
// __foe__: typeof foe;
Comment on lines +37 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 analysis of the import patterns and dependencies, I can now provide a final response:

Externalization blockers likely due to internal package dependencies

The investigation reveals that these modules cannot be externalized due to complex interdependencies within the codebase:

  • @fiftyone/core is heavily imported across multiple packages and contains shared components
  • @fiftyone/looker-3d has bidirectional dependencies with core (e.g., core imports Looker3d component while looker-3d imports core components)
  • The modules are tightly coupled through direct imports of internal components (e.g., @fiftyone/core/src/plugins/SchemaIO/components/ButtonView)
  • There are deep imports accessing specific internal paths rather than public APIs

These patterns suggest that externalization would require significant refactoring to:

  1. Establish clear public APIs
  2. Remove direct imports of internal components
  3. Resolve the circular dependencies between packages
🔗 Analysis chain

Investigate root cause of externalization failures.

The TODO comments indicate systematic issues with specific modules (focore, fol3d, fom, foe). This could be related to circular dependencies or module resolution problems.

Let's check for potential circular dependencies:

Let me gather more specific information about the imports and dependencies between these modules.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential circular dependencies in problematic modules

# Search for cross-references between these modules
echo "Checking for cross-references between problematic modules..."
rg -l "@fiftyone/(core|looker-3d|map|embeddings)" --type ts

# Check import patterns
echo "Analyzing import patterns..."
ast-grep --pattern 'import * from "@fiftyone/$_"'

Length of output: 1169

Script:

#!/bin/bash
# Check for specific imports and dependencies between the problematic modules

# Look for direct imports of these modules
echo "Checking direct imports..."
rg "from ['\"]@fiftyone/(core|looker-3d|map|embeddings)" -B1 -A1

# Check for type references
echo -e "\nChecking type references..."
rg "type.*(@fiftyone/(core|looker-3d|map|embeddings))" --type ts

# Look for circular import patterns
echo -e "\nChecking for potential circular patterns..."
for module in core looker-3d map embeddings; do
  echo "=== Checking $module ==="
  rg "from.*@fiftyone/$module" app/packages/$module/
done

Length of output: 5409

}
}

Expand All @@ -58,13 +56,14 @@ if (typeof window !== "undefined") {
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;
window.__styled__ = styled;
// todo: the following cannot be externalized because of unknown reason
// window.__fol3d__ = fol3d;
// window.__foe__ = foe;
// window.__fom__ = fom;
// window.__focore__ = focore;
Comment on lines +64 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 runtime checks for externalized dependencies.

Since some modules cannot be externalized, consider adding runtime checks to gracefully handle cases where plugins might attempt to access these unavailable modules.

Add defensive checks:

if (typeof window !== "undefined") {
+  const warnUnavailable = (module: string) => {
+    console.warn(`${module} is not available as an external dependency`);
+  };
+
   // existing assignments...
-  // window.__fol3d__ = fol3d;
+  Object.defineProperty(window, '__fol3d__', {
+    get() {
+      warnUnavailable('looker-3d');
+      return undefined;
+    }
+  });
   // Similar pattern for other unavailable modules
}

Committable suggestion was skipped due to low confidence.

}
Loading
Loading