adds a polyfill for iterator-helpers #8877
Conversation
📝 WalkthroughWalkthroughAdds two top-level polyfill imports for iterator helpers and Set methods to the frontend entry point and introduces Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
How can we test this? Does |
BTW, we have a browser check for the iterator helpers here: Did those people at least see our warning to update their browser? |
It did show up. I don't think they would update their browser just for WK, though. Sometimes they can't do that on their own, because of IT policies. |
Maybe we should relax our browser targets. We don't rely on terribly new features apart for code convenience, no? Maybe we should switch to "defaults" and add the relevant polyfills. |
In case of the iterator helpers, it is a performance improvement more than code convenience. Not sure, what browser features we need for WebGL...
Isn't that the whole point of using browser list that the build tools automatically add polyfills? |
Well, a function* flatMap(it, func) {
for (const value of it) {
const mappedValues = func(value);
yield* mappedValues;
}
}That is not a feature that just the runtime can implement, like a new data strucure or syntax. So, it falls in the code convienence category.
Nothing recent comes to mind.
I think that would be much better than "last 3 versions".
Esbuild doesn't inject polyfills that and recommends the use of core-js. Since we are not using babel, I don't think we have any polyfills in there now. |
![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)
frontend/javascripts/main.tsx (1)
1-1: No explicit iterator-helper usage detected; reconsider this polyfill and its placement.Our search across frontend/javascripts TS/TSX files found no direct calls to Iterator.from or any Iterator.prototype.* helpers. If your code isn’t actually using these iterator helpers, you may not need this import at all.
However, we did discover an additional entry point in frontend/javascripts/libs/DRACOLoader.ts (line 241) which spawns a web worker for DRACOWorker.worker.js. Polyfills loaded in main.tsx won’t apply inside that worker, potentially leading to runtime errors if the worker ever uses iterator helpers.
To ensure both correct feature detection and broad runtime support:
- Consider removing the import entirely if iterator helpers aren’t in use.
- Or centralize iterator polyfills in a shared polyfills.ts that’s imported before app initialization (and included in your worker bundles).
- Alternatively, update libs/browser_feature_check to distinguish native vs. polyfilled support so that your “outdated browser” warning isn’t masked by a polyfill.
Let me know which approach you’d prefer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
frontend/javascripts/main.tsx(1 hunks)package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
| "classnames": "^2.2.5", | ||
| "color-hash": "^2.0.1", | ||
| "comlink": "^4.3.0", | ||
| "core-js": "^3.45.1", |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Runtime dep looks good; ensure the module path exists in this version.
Adding core-js as a production dependency is appropriate for runtime polyfills.
Run to verify the imported path resolves with the chosen version:
🏁 Script executed:
#!/bin/bash
# Confirm the iterator helpers entry exists in installed core-js
node -e "console.log(require.resolve('core-js/actual/iterator'))"Length of output: 838
Invalid core-js import path requires correction
Running
node -e "console.log(require.resolve('core-js/actual/iterator'))"on core-js@3.45.1 throws MODULE_NOT_FOUND, which means the entry point you’re importing doesn’t exist at runtime. You must update the import to a valid module path to avoid breaking your build.
• Locate every occurrence of
require('core-js/actual/iterator')(or the equivalent ES module import) in your code.
• If you need the stage-3 iterator helpers proposal, replace it with:
-require('core-js/actual/iterator')
+require('core-js/proposals/iterator-helpers')• If you only need core JS’s standard iterator polyfills, use one of the supported entry points, for example:
require('core-js/features/iterator');
// or
require('core-js/modules/es.array.iterator');After updating, verify that the new path resolves correctly:
node -e "console.log(require.resolve('core-js/proposals/iterator-helpers'))"🤖 Prompt for AI Agents
package.json around line 145: the project depends on core-js@3.45.1 but code
imports the non-existent entry point "core-js/actual/iterator", which causes
MODULE_NOT_FOUND at runtime; search the repo for any occurrences of
require('core-js/actual/iterator') or equivalent ESM imports and replace them
with a valid entry such as 'core-js/features/iterator' or
'core-js/modules/es.array.iterator' (or if you need the stage-3 proposal use
'core-js/proposals/iterator-helpers' and ensure that proposal package is
available), then run node -e "console.log(require.resolve('<chosen-path>'))" to
confirm resolution and update any build/config files accordingly.
…ebknossos into iterator-helper-polyfill
Adds a polyfill for iterator-helpers, because some people haven't upgraded their browsers yet.