sadpandajoe commented on code in PR #45244:
URL: https://github.com/apache/superset/pull/45244#discussion_r4284610233
##########
superset-extensions-cli/src/superset_extensions_cli/cli.py:
##########
@@ -108,14 +108,17 @@ def init_frontend_deps(frontend_dir: Path) -> None:
"""
node_modules = frontend_dir / "node_modules"
if not node_modules.exists():
+ # The published @apache-superset/core still pins react/react-dom peers
to
+ # ^17.0.2, while scaffolded extensions target the host's actual
^18.3.0 --
+ # a plain install hits ERESOLVE until core's peer range is widened.
package_lock = frontend_dir / "package-lock.json"
if package_lock.exists():
click.secho("⚙️ node_modules not found, running `npm ci`…",
fg="cyan")
- npm_command = ["npm", "ci"]
+ npm_command = ["npm", "ci", "--legacy-peer-deps"]
Review Comment:
Nothing exercises this `npm ci` branch: neither install test creates a
`package-lock.json`, and `test_init_frontend_deps_exits_on_npm_ci_failure`
actually runs `npm i`. If someone drops `--legacy-peer-deps` here, the lockfile
path hits ERESOLVE again and the suite stays green. Could you add a case with a
lockfile present that asserts `["npm", "ci", "--legacy-peer-deps"]`?
##########
superset-extensions-cli/src/superset_extensions_cli/cli.py:
##########
@@ -108,14 +108,17 @@ def init_frontend_deps(frontend_dir: Path) -> None:
"""
node_modules = frontend_dir / "node_modules"
if not node_modules.exists():
+ # The published @apache-superset/core still pins react/react-dom peers
to
+ # ^17.0.2, while scaffolded extensions target the host's actual
^18.3.0 --
+ # a plain install hits ERESOLVE until core's peer range is widened.
package_lock = frontend_dir / "package-lock.json"
if package_lock.exists():
click.secho("⚙️ node_modules not found, running `npm ci`…",
fg="cyan")
- npm_command = ["npm", "ci"]
+ npm_command = ["npm", "ci", "--legacy-peer-deps"]
error_msg = "❌ `npm ci` failed. Aborting."
else:
click.secho("⚙️ node_modules not found, running `npm i`…",
fg="cyan")
- npm_command = ["npm", "i"]
+ npm_command = ["npm", "i", "--legacy-peer-deps"]
Review Comment:
`init_frontend_deps` also backs `build` and `dev` for existing extensions,
and `--legacy-peer-deps` ignores peer dependencies entirely rather than only
the React conflict. An extension relying on npm to auto-install a peer (e.g.
`react-chartjs-2` pulling in `chart.js`) would stop getting it on a clean
install and fail to build. Is there a way to scope the workaround to the
scaffold (for example an `overrides` entry in the template) instead of changing
install semantics for every extension?
##########
superset-extensions-cli/src/superset_extensions_cli/templates/frontend/package.json.j2:
##########
@@ -15,13 +15,14 @@
"description": "",
"peerDependencies": {
"@apache-superset/core": "*",
- "react": "^17.0.2",
- "react-dom": "^17.0.2"
+ "react": "^18.3.0",
+ "react-dom": "^18.3.0"
},
"devDependencies": {
+ "@apache-superset/core": "^0.1.0",
Review Comment:
The CLI path is covered now, but a direct `npm install` in the scaffolded
`frontend/` still fails with ERESOLVE, and quick-start step 6
(`docs/developer_docs/extensions/quick-start.md`) tells users to run exactly
that. The same happens for any later `npm install <pkg>`. Would a scaffolded
`.npmrc` with `legacy-peer-deps=true` close that gap, rather than a flag only
the CLI wrapper passes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
