fix(app): normalize subpath imports to install base npm package#6380
fix(app): normalize subpath imports to install base npm package#6380BABTUNA wants to merge 2 commits intoreflex-dev:mainfrom
Conversation
Greptile SummaryThis PR fixes frontend package installation for components that use npm subpath imports (e.g.
Confidence Score: 3/5Not safe to merge as-is — introduces a regression for https:// URL imports that was absent before this change. The P1 finding is a concrete behavioural regression: any component whose import key starts with https:// will now attempt to install a package named 'https:' instead of the original URL. This replaces working behaviour with a silent mis-install. The P2 versioned-subpath edge case is lower risk but also untested. reflex/app.py — _get_frontend_package_name needs an early-return for https:// URL imports and a fix for the versioned+subpath case. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["import_name"] --> B{"empty or starts\nwith /, $/, .?"}
B -- yes --> C["return None"]
B -- no --> D["format_library_name(import_name)\n→ strips @version suffix"]
D --> E{"starts with '@'?\n(scoped package)"}
E -- yes --> F["Extract @scope/pkg\nfrom @scope/pkg/subpath"]
E -- no --> G["Split on first '/'\n→ base package name"]
F --> H{"package_name\n== library_name?"}
G --> H
H -- yes --> I["return import_name\n⚠️ BUG if versioned+subpath\n⚠️ BUG if https:// URL"]
H -- no --> J{"import_name has\npinned version?"}
J -- yes --> K["return package_name@version"]
J -- no --> L["return package_name"]
Reviews (1): Last reviewed commit: "fix(app): install base npm package for s..." | Re-trigger Greptile |
| package_name = library_name.split("/", maxsplit=1)[0] | ||
|
|
||
| if package_name == library_name: | ||
| return import_name |
There was a problem hiding this comment.
https:// URL imports mangled to "https:"
format_library_name short-circuits for https://… URLs and returns the full URL unchanged. That URL then falls through to the unscoped branch where .split("/", maxsplit=1)[0] produces "https:", which is returned as the package name and later passed to install_frontend_packages.
The original code passed such URL imports through intact; this PR introduces a regression for any component whose import key starts with https:// (e.g. Skypack CDN imports). An early-return for https:// in _get_frontend_package_name would restore the previous behaviour:
if import_name == "" or any(
import_name.startswith(prefix) for prefix in ("/", "$/", ".", "https://")
):
return None| if package_name == library_name: | ||
| return import_name |
There was a problem hiding this comment.
Versioned + subpath imports returned unchanged
When an import carries both a version pin and a subpath, e.g. react-map-gl@1.0.0/maplibre, format.format_library_name strips @1.0.0/maplibre entirely via rpartition("@"), yielding library_name = "react-map-gl". At that point package_name (also "react-map-gl") equals library_name, so the guard if package_name == library_name: return import_name fires and returns "react-map-gl@1.0.0/maplibre" — the original subpath-containing string — defeating the fix.
The condition is meant to short-circuit when there is no subpath to strip, but it does not account for format_library_name having already consumed the subpath as part of the version segment. Checking whether the original import_name contains a / after the version separator would close the gap.
|
Addressed both review points in 7516516:
Added regression tests:
Validation:
|
All Submissions:
Type of change
Changes To Core Features:
Description
Fixes frontend package installation when component import keys include package subpaths.
Before this change, _get_frontend_packages attempted to install subpath keys directly (for example
eact-map-gl/maplibre), which is not an installable npm package name.
This patch adds package-name normalization in App._get_frontend_packages:
eact-map-gl/maplibre to
eact-map-gl.
Tests
Added regression test:
Validation run:
Closes #6234