Serve the alternate UI framework locally instead of from a CDN - #288
Draft
evnchn wants to merge 1 commit into
Draft
Serve the alternate UI framework locally instead of from a CDN#288evnchn wants to merge 1 commit into
evnchn wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Drafted by Claude Code on evnchn's behalf.
Fork staging PR — for review before anything goes upstream. Carries a judgement call worth a maintainer's opinion — see "Decision for review" below.
Motivation
tests/test_alternate_ui_frameworks.py::test_element_plusfetchedhttps://unpkg.com/element-plusat test time. It was the only test in the suite with an external network dependency, and it fails in the merge queue when that fetch is slow.Observed (run 30927252265, py3.11) — the page load itself stalls, so it dies before any assertion:
Reproducing it locally with CDP network blocking surfaced two distinct failure modes, not one:
TimeoutException(what CI hit)el-buttonnever upgrades →NoSuchElementExceptionand in both cases the console logs
Uncaught ReferenceError: ElementPlus is not defined, whichscreen_plugin.pyturns into a job failure independently of the test result.Implementation
Serve the alternate framework from the local test server instead of a CDN:
Deliberately kept as a
<script defer>external script, not an inline one, so the test still exercises the ordering that matters — thatvue_config_script'sapp.use(...)runs after a deferred external bundle has loaded. An inline stub would have quietly dropped that coverage. The rest of the path (add_body_html→vue_config_script→ custom element renders) is unchanged.Follows the existing precedent in
tests/test_aggrid.py::test_set_module_source, which already serves a JS bundle from a test route.Verification
It passes, and — more importantly — it can still fail. A stub test that is vacuously green would be worse than the flake it replaces, so I sabotaged the line under test:
Offline by construction — the only URL referenced is the relative
/alternate-ui.js; no external host is contacted. Runtime drops to ~1.8 s with no network variance.Gates: pre-commit all Passed · mypy clean over 245 source files · pylint 10.00/10.
The approach I tried first, and why I abandoned it
My first attempt kept the real CDN and skipped the test when it was unavailable:
It worked — verified
1 skippedwith the CDN blocked and2 passedwith it up, including whitelisting the console error that would otherwise fail teardown regardless of the skip.Rejected anyway.
window.ElementPlus === undefineddoes not only mean "CDN down" — it is also what you get if unpkg changes the bundle shape, or if a NiceGUI regression breaksadd_body_html/vue_config_scriptordering. The skip would convert a real regression into permanently green CI that tests nothing. Trading a loud flake for a silent hole is a bad trade. (Raised as a MUST-FIX by a second-lineage review; I agree with it.)Decision for review
This removes CI's dependence on unpkg.com, at the cost of no longer proving that a real third-party framework works — it proves NiceGUI's plumbing works with an external deferred bundle, using a synthetic one.
If that coverage is wanted, the alternatives are:
test_element_plusas an opt-in test excluded from the default CI run (e.g. behind a marker), so the offline test guards every run and the real-CDN one is available on demand.Happy to switch to either; option 2 is a small addition on top of this PR.
Progress