fix: Replace instanceof Array with Array.isArray()#10222
fix: Replace instanceof Array with Array.isArray()#10222Moumouls wants to merge 3 commits intoparse-community:alphafrom
instanceof Array with Array.isArray()#10222Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. |
Moumouls
changed the title
📝 WalkthroughWalkthroughThis PR replaces uses of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
I will reformat the title to use the proper commit message syntax. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## alpha #10222 +/- ##
==========================================
- Coverage 92.18% 92.18% -0.01%
==========================================
Files 192 192
Lines 16307 16307
Branches 199 199
==========================================
- Hits 15033 15032 -1
- Misses 1253 1254 +1
Partials 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
spec/DatabaseController.spec.js (1)
60-85: Refactor these new tests to use async/await instead of done callbacks.The three cross-realm array tests (lines 60, 69, 78) are solid, but they use the
donecallback pattern. New tests in this suite should useasync/awaitto align with the established promise-based pattern.Refactoring pattern
- it('should accept cross-realm arrays for $or', done => { + it('should accept cross-realm arrays for $or', async () => { const query = { $or: vm.runInNewContext('[{ a: 1 }, { b: 2 }]'), }; expect(Array.isArray(query.$or)).toBe(true); expect(() => validateQuery(query)).not.toThrow(); - done(); }); - it('should accept cross-realm arrays for $and', done => { + it('should accept cross-realm arrays for $and', async () => { const query = { $and: vm.runInNewContext('[{ a: 1 }, { b: 2 }]'), }; expect(Array.isArray(query.$and)).toBe(true); expect(() => validateQuery(query)).not.toThrow(); - done(); }); - it('should accept cross-realm arrays for $nor', done => { + it('should accept cross-realm arrays for $nor', async () => { const query = { $nor: vm.runInNewContext('[{ a: 1 }]'), }; expect(Array.isArray(query.$nor)).toBe(true); expect(() => validateQuery(query)).not.toThrow(); - done(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/DatabaseController.spec.js` around lines 60 - 85, Convert the three tests that currently use the done callback into async functions: change each it(..., done => { ... done(); }) to it(..., async () => { ... }) and remove the done parameter and final done() call; keep the existing bodies (vm.runInNewContext('[...]'), Array.isArray assertions, and the expect(() => validateQuery(...)).not.toThrow() checks) unchanged so they remain synchronous but use the async/await test style; apply this to the tests referencing $or, $and, and $nor and the usages of vm.runInNewContext and validateQuery.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Controllers/DatabaseController.js`:
- Around line 134-152: The three forEach callbacks iterating query.$or,
query.$and, and query.$nor currently use expression-bodied arrow functions that
implicitly return the result of validateQuery; change them to block-bodied
callbacks that call validateQuery without returning (e.g., replace value =>
validateQuery(...) with value => { validateQuery(...); }) for each occurrence so
the linter rule lint/suspicious/useIterableCallbackReturn is satisfied and
behavior is unchanged.
---
Nitpick comments:
In `@spec/DatabaseController.spec.js`:
- Around line 60-85: Convert the three tests that currently use the done
callback into async functions: change each it(..., done => { ... done(); }) to
it(..., async () => { ... }) and remove the done parameter and final done()
call; keep the existing bodies (vm.runInNewContext('[...]'), Array.isArray
assertions, and the expect(() => validateQuery(...)).not.toThrow() checks)
unchanged so they remain synchronous but use the async/await test style; apply
this to the tests referencing $or, $and, and $nor and the usages of
vm.runInNewContext and validateQuery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f64eb102-16ef-4bf9-8802-25bbe234df7d
📒 Files selected for processing (17)
eslint.config.jsspec/DatabaseController.spec.jsspec/MongoStorageAdapter.spec.jsspec/ParseAPI.spec.jsspec/eslint.config.jsspec/helper.jsspec/rest.spec.jssrc/Adapters/Storage/Mongo/MongoTransform.jssrc/Adapters/Storage/Postgres/PostgresStorageAdapter.jssrc/Config.jssrc/Controllers/DatabaseController.jssrc/Controllers/FilesController.jssrc/Controllers/LiveQueryController.jssrc/Controllers/SchemaController.jssrc/GraphQL/transformers/query.jssrc/RestQuery.jstypes/eslint.config.mjs
mtrezza
changed the title
instanceof Array with Array.isArray()
There was a problem hiding this comment.
This PR fix is too narrow. The issue stems from the fact that instanceof is not realm-safe, regardless of which type it compares to. So we should remove all occurrences of instanceof throughout the codebase. I would prefer a more structured approach. Introducing helper functions would be a better approach, especially as some of the realm-safe alternatives are more complex. Use the existing Utils.js and add isDate(), isRegex(), etc. It seems that Node.js is moving towards providing safe alternatives, but for example Date.isDate() is still in proposal stage it seems.
|
Superseded by #10225, which covers all |
3 participants
Pull Request
Issue
instanceof Array is not realm safe, so in test frameworks like Jest or in advanced parse-server setup, many queries and parse-server features can break.
Approach
Replacing instanceof Array with Array.isArray() which is realm safe, and official MDN recommendation
Added an eslint rule to prevent futur problem related to this kind of issue
Tasks
Summary by CodeRabbit
Bug Fixes
Tests
Chores