fix externalize bug #4984
fix externalize bug #4984
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces changes to two files within the project. In 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/packages/plugins/src/externalize.ts (1)
Line range hint
1-68: Consider documenting the externalization strategy.The current approach of selectively externalizing modules suggests there might be architectural constraints or patterns that should be documented. This would help future maintainers understand:
- Which modules can be safely externalized
- What conditions must be met for successful externalization
- Common pitfalls and their solutions
Consider creating a
EXTERNALIZATION.mddocument in the project root to capture these learnings and guidelines.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
app/packages/looker-3d/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (2)
- app/packages/looker-3d/src/index.ts (1 hunks)
- app/packages/plugins/src/externalize.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/looker-3d/src/index.ts
🧰 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)
2-8: LGTM: Import changes align with externalization requirements.The new imports are properly structured and follow consistent naming patterns.
| // todo: the following cannot be externalized because of unknown reason | ||
| // window.__fol3d__ = fol3d; | ||
| // window.__foe__ = foe; | ||
| // window.__fom__ = fom; | ||
| // window.__focore__ = focore; |
There was a problem hiding this comment.
🛠️ 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.
| // todo: the following cannot be externalized because of unknown reason | ||
| // __focore__: typeof focore; | ||
| // __fol3d__: typeof fol3d; | ||
| // __fom__: typeof fom; | ||
| // __foe__: typeof foe; |
There was a problem hiding this comment.
💡 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/coreis heavily imported across multiple packages and contains shared components@fiftyone/looker-3dhas 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:
- Establish clear public APIs
- Remove direct imports of internal components
- 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
#4970 introduced changes that externalized more FO packages to be used in JS plugins. However, some of the packages externalized is causing the app to not build (specifically,
@fiftyone/pluginmodule is not being resolved properly). The reason is unknown and will be investigated later.This PR removes the culprit packages. This is a fine trade-off for now since these packages aren't expected to be used in JS plugins.
Summary by CodeRabbit
New Features
Looker3dmodule, enhancing accessibility for users.Bug Fixes