fix: js library default export to a unique name instead of default key in self#36483
fix: js library default export to a unique name instead of default key in self#36483AmanAgarwal041 merged 6 commits intoreleasefrom
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
Poem
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
app/client/src/workers/Evaluation/handlers/jsLibrary.ts (1)
151-155: UseforEachinstead ofmapfor side effects.Since we're performing actions for their side effects and not utilizing the returned array from
map, it's better to useforEach.Apply this diff to make the change:
- differentiatingKeys.map((key) => { + differentiatingKeys.forEach((key) => {
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] 149-149: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10995172142. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/src/workers/Evaluation/handlers/__tests__/jsLibrary.test.ts (1)
164-186: Excellent test case, but let's add a gold star to it!Well done on crafting this test case, students! It's a wonderful addition to our test suite. Let's review what you've done right:
- You're testing the installation of the jspdf-autotable library, which is the focus of our lesson.
- You're verifying that
importScriptsis called, ensuring our mock is utilized.- You're checking the structure of the installation response, including the accessor and definitions.
However, to earn that extra credit, let's make one small improvement:
Consider adding an assertion to check if the
Cellfunction is available on the installed library. This will ensure that our flattening process for default exports is working correctly. Here's how you can do it:expect(self.jspdf_plugin_autotable_js.Cell).toBeDefined();Add this assertion just before the end of the test case. This will give us full confidence that our library is installed and accessible as expected.
Keep up the great work, class!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/workers/Evaluation/handlers/tests/jsLibrary.test.ts (2 hunks)
- app/client/src/workers/Evaluation/handlers/jsLibrary.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/workers/Evaluation/handlers/jsLibrary.ts
Additional comments not posted (2)
app/client/src/workers/Evaluation/handlers/__tests__/jsLibrary.test.ts (2)
14-24: Class, pay attention to this excellent mock implementation!Well done, students! This mock implementation of
importScriptsis a shining example of how to prepare our test environment. Let's break it down:
- We're checking if the URL includes "jspdf-autotable". This is crucial for our specific test case.
- We're setting up a mock structure that mimics the real library's behavior.
- We're attaching both a default export and a
Cellfunction to the globalselfobject.This change is vital for our lesson on testing library installations, especially those with default exports. It allows us to simulate the behavior of the jspdf-autotable library accurately.
Remember, class: proper mocking is key to effective unit testing!
Line range hint
1-186: Class, let's summarize today's lesson!Excellent work on this test file, students! You've made significant improvements that directly address our lesson objectives. Let's recap what we've learned:
- We've enhanced our mock implementation to better simulate libraries with default exports, particularly the jspdf-autotable library.
- We've added a new test case that verifies the correct installation and accessibility of such libraries.
These changes are crucial in ensuring that our library installation process works correctly, especially for libraries that use default exports. This will help prevent the namespace issues we've been struggling with.
For homework, consider adding more test cases to cover different scenarios, such as:
- Installing multiple versions of the same library
- Handling conflicts with existing global objects
- Testing error cases, like network failures during installation
Remember, thorough testing is the key to robust code! Keep up the great work, and don't forget to submit your improved tests for extra credit!
|
Deploy-Preview-URL: https://ce-36483.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10996267004. |
|
Deploy-Preview-URL: https://ce-36483.dp.appsmith.com |
|
@AmanAgarwal041 I tried to reinstall the library after uninstalling it and I was able to reproduce the issue. I demoed the same in the recording below. Could you handle this case and add a test for it? |
|
@rishabhrathod01 Addressed the issue. It was acting weird because of the |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11011543756. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
app/client/src/workers/Evaluation/handlers/jsLibrary.ts (2)
136-163: Excellent work on handling the "default" export, but let's improve our performance!Your approach to handling the "default" export and other differentiating keys is spot on! It effectively addresses the issue mentioned in the PR objectives. However, we can make a small improvement:
On line 150, instead of using the
deleteoperator, which can impact performance, let's set the property toundefined. This is a more efficient way to remove a property from an object.Apply this change:
- delete self["default"]; + self["default"] = undefined;Keep up the great work, and always remember to optimize for performance!
Tools
Biome
[error] 150-150: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
136-163: Let's add a helpful comment to explain this code block.To help your fellow students (I mean, developers) understand the purpose of this code block, it would be beneficial to add a comment explaining what it does. Here's a suggestion:
// Handle the case where the library has a default export // If present, map it to a unique name and reassign other exports // If not present, simply add all new keys to the accessors arrayAdding such comments makes your code more readable and easier to maintain. It's like leaving good notes for the next person who reads your code!
Tools
Biome
[error] 150-150: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
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] 150-150: 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)
131-133: Well done, class! This is a great way to identify new keys.The use of the
differencefunction to computedifferentiatingKeysis an excellent approach. It efficiently identifies the new keys added by the library installation. Keep up the good work!
|
Deploy-Preview-URL: https://ce-36483.dp.appsmith.com |
2 participants
Description
Earlier since some of the libraries one of which is
https://cdn.jsdelivr.net/npm/jspdf-autotable@3.5.28/dist/jspdf.plugin.autotable.jswasn't exporting the functionality to some key, instead it was exporting as default. Hence, after the import of the library the functionality was being set on the default key, which on refresh or on reintializing the application in the store got changed and the inner functions got attached to global object.This PR fixes this issue and checks if the last accessor is the
defaultthen it maps all the functionality to a unique name generated from the library url and moves everything to the generated name as done by other libraries.Fixes #20572
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/11011513239
Commit: b4c0767
Cypress dashboard.
Tags:
@tag.JSSpec:
Tue, 24 Sep 2024 10:40:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes