fix(browser): normalize wait timeouts#655
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
The 36kr wait improvement and the sinafinance fix both make sense. One thought on the approach:
WaitOptions.timeout is already defined as seconds, and only sinafinance/rolling-news.ts passes the wrong unit (10000 instead of 10). The simpler fix would be to just correct that one caller rather than introducing a unit-guessing function. Auto-detecting "is this seconds or milliseconds?" based on a >= 1000 threshold adds an implicit convention that could confuse future contributors — someone passing timeout: 1000 (1000 seconds) would silently get 1 second instead.
Enforcing one consistent unit (seconds, as already documented) and fixing the one wrong call site keeps the codebase simpler.
hiSandog
left a comment
There was a problem hiding this comment.
Good point — I agree keeping a single consistent unit (seconds) is simpler and less error-prone. I reverted the normalizeWaitTimeoutMs helper entirely and kept the original (timeout ?? default) * 1000 pattern. The only functional changes are:
- 36kr/article: improved wait to use waitForCapture + selector wait instead of a blind sleep
- sinafinance/rolling-news: fixed the wrong unit from 10000 (ms) → 10 (s)
Tests pass. Thanks for the review!
8d866e5 to
45001ec
Compare
Summary
Testing
RUN v4.1.1 /Users/sandog/sandog/code/kaiyuan-space/opencli
Test Files 3 passed (3)
Tests 13 passed (13)
Start at 14:46:42
Duration 349ms (transform 82ms, setup 0ms, import 113ms, tests 259ms, environment 0ms)
RUN v4.1.1 /Users/sandog/sandog/code/kaiyuan-space/opencli
Test Files 2 passed (2)
Tests 4 passed (4)
Start at 14:46:42
Duration 79ms (transform 36ms, setup 0ms, import 42ms, tests 10ms, environment 0ms)