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
High-level overview: - Replace the `webpack` frontend builds with `vite` - Internally, [Typescript and JSX compilation](https://vite.dev/guide/features#typescript) are still handled through `esbuild` - Internally, JS bundling is handled by `rollup`(soon to be replace with it's Rust re-write 'rolldown') - Internally, CSS/LESS compilation is handled by [PostCSS](https://github.com/postcss/postcss) - Updated `vitest` test runners to use the base `vite` config. In other words, tests and production build should use the same plugins, settings, transformations (e.g. protobuf handling), package versions, etc. - `vite` treats the `index.html` file as the main starting point. I moved the `index.html.scala` out of the `Play` into a separate `<root>/index.html` file, without backend templating. - Added a small, custom vite plugin for handling `protobuf` files. Essentially, it does the same protobuf-to-JSON conversions as previously provided by `global_mocks.ts`. - `vite` is configured to a [broader baseline of Browser version](https://vite.dev/guide/build#browser-compatibility) for JS compatibility. (related to #8877 ) It does not, however, include polyfills: ``` Chrome >=107 Edge >=107 Firefox >=104 Safari >=16 ``` - added a new `<FailedToLoadView>` React view, when WK is unable to initialized. Most often this is the case, when the backend is still compiling and not yet responding. Inspired by `proxy.js` waiting/error page. ### Asset Management - All static frontend assets (images, wasm-files) have been moved from `/public` to `/frontend/assets`. In other words, frontend asset handling was moved from `Play framework` to `vite`. - During the build process, vite copies all managed assets together with the bundled JS in the `/public` folder. Play will then serve them as static files. - Ideally, one could import every image in the frontend code, e.g. import MyImage from "images/my_image.png`. This would guarantee that files are checked to named correctly/available. I have not YET done that in this PR. I configured vite configured `vite? to use frontend/assets as an asset directory. Another use case, - ### Dev Environment - `vite` ships with a builtin dev server incl. hot reloading - In dev mode, JS is is not bundled but rather served as raw ES modules - Frontend asset, ie. JS, images, wasm are all served by the vite dev server - the vite dev server has a built-in proxy to forward API calls to the backend - Updated the `yarn start` commands to launch WK backend, fossildb and vite dev server simultaneously - ### Production Builds - `vite build` does the JS compilation, bundling and asset copying to `/public`. - `Play` serves all assets from the `public` directory (incl. `index.html`) - `Play` inserts `Airbrake` credentials, `OpenGraph` metadata etc into `index.html` as before ### URL of deployed dev instance (used for testing): - https://vite.webknossos.xyz ### Steps to test: - Run `yarn install` and `yarn start` to test dev mode - Run a full vite build (`yarn build`). Enable `vite-build-analyzer` (see commented out analyzer in`vite.config.js`) and inspect bundle splits. - Visit https://vite.webknossos.xyz to test production mode - Verify web workers being initialized ### TODOs: - [x] Fix type errors - [x] Fix unit tests - [x] Fix all the backend template stuff from index.html.scala that is not available in `index.html` - [ ] ~~Add DB migration of organization logo URI~~ ### Issues: - fixes #9186 ------ (Please delete unneeded items, merge only when none are left open) - [x] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`) - [ ] Added migration guide entry if applicable (edit the same file as for the changelog) - [ ] Updated [documentation](../blob/master/docs) if applicable - [ ] Adapted [wk-libs python client](https://github.com/scalableminds/webknossos-libs/tree/master/webknossos/webknossos/client) if relevant API parts change - [ ] Removed dev-only changes like prints and application.conf edits - [ ] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) - [ ] Needs datastore update after deployment --------- Co-authored-by: Florian M <florian@scm.io> Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
2 participants
Adds a polyfill for iterator-helpers, because some people haven't upgraded their browsers yet.