fix: js library load functionality handling for default exports #36539
Conversation
WalkthroughThe changes in this pull request enhance the handling of default exports in the Changes
Assessment against linked issues
Possibly related PRs
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
|
github-actions
bot
added
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11032788578. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/workers/Evaluation/handlers/jsLibrary.ts (1)
325-331: Use 'forEach' Instead of 'map' for Iteration with Side EffectsIn this block, you're using
mapto iterate overdefaultAccessorsand perform operations, butmapis intended to create a new array based on the return values. Since the returned array isn't used,forEachis more appropriate for iterating when you're not collecting results.Consider changing the code to:
-defaultAccessors.map((key) => { +defaultAccessors.forEach((key) => {This change clarifies your intent and avoids confusion about unused return values.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/workers/Evaluation/handlers/jsLibrary.ts (1 hunks)
🧰 Additional context used
Biome
app/client/src/workers/Evaluation/handlers/jsLibrary.ts
[error] 323-323: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments not posted (1)
app/client/src/workers/Evaluation/handlers/jsLibrary.ts (1)
323-323:⚠️ Potential issueConsider Avoiding the 'delete' Operator for Performance
Using the
deleteoperator can negatively impact performance because it alters the object's structure, leading to de-optimization in JavaScript engines. Instead, assigningundefinedto the property maintains the object's shape and is more efficient.Let's modify the code as follows:
-delete self["default"]; +self["default"] = undefined;⛔ Skipped due to learnings
Learnt from: rishabhrathod01 PR: appsmithorg/appsmith#36483 File: app/client/src/workers/Evaluation/handlers/jsLibrary.ts:152-158 Timestamp: 2024-09-24T10:45:10.162Z Learning: Replacing `delete self[key];` with `self[key] = undefined;` in the `installLibrary` function of `jsLibrary.ts` causes bugs in the product. Therefore, we should continue using the `delete` operator when removing properties from `self`.🧰 Tools
Biome
[error] 323-323: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
|
Deploy-Preview-URL: https://ce-36539.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/workers/Evaluation/handlers/jsLibrary.ts (3 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/workers/Evaluation/handlers/jsLibrary.ts
[error] 463-463: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (2)
app/client/src/workers/Evaluation/handlers/jsLibrary.ts (2)
131-155: Well done on improving the handling of default exports!Class, let's take a moment to appreciate the improvements made to the
installLibraryfunction. The changes here show a good understanding of how to handle default exports more effectively.
- The logic for determining differentiating keys has been streamlined, making it easier to follow.
- The new function
movetheDefaultExportedLibraryToAccessorKeyencapsulates the logic for mapping default exports to unique accessor names, which is a great example of code organization.- By reassessing the differentiating keys after the default export operation, we ensure that all changes are properly accounted for.
These modifications will make our library installation process more robust and easier to maintain. Keep up the good work!
303-314: Excellent work on maintaining consistency!Class, let's examine the improvements made to the
loadLibrariesfunction. It's crucial to maintain consistency across related functions, and that's exactly what has been achieved here.
- The handling of default exports now mirrors the approach used in the
installLibraryfunction.- By calling
movetheDefaultExportedLibraryToAccessorKey, we ensure that default exports are handled uniformly throughout the codebase.- The reassessment of default accessors after the operation maintains the integrity of our library loading process.
This consistency will make our code more predictable and easier to understand. Remember, consistency is key in writing maintainable code!
Description
When the library which exports as
defaultwas installed, it was being handled to add the functionality to the new accessor name generated from theurl. This was fixed from the pr : #36483When the application page is refreshed the installed library get loaded again, which was again installing the library to
defaultaccessor. This PR handles the accessor to be which was already assigned while installing the library.Fixes #36530
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.JS"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11049788382
Commit: 1cf092e
Cypress dashboard.
Tags:
@tag.JSSpec:
Thu, 26 Sep 2024 10:46:34 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit