web: extract shared code into web-common.js - #961
Conversation
|
@Fristender please be patient, I will get to this but it might be a week or two. @MichaelBelousov Can you read this PR description and tell me if splitting the js across multiple files might cause any problems? |
|
Ofc, will take a look later! |
|
@david-vanderson I can take over as reviewer for this if you are Ok with that |
That would be amazing! This PR is not supposed to change any behavior, just a refactor. |
|
Unrelated, @Didas-git it looks like #706 broke web.d.ts for some users (like myself). I didn't realize because I'm still behind on a version (and I don't always run the typecheck rn lol). The web.d.ts should contain the list of public functions from web.js. Why was the exported interface removed? I am a user of that interface. On second thought, let's move discussion to that PR. |
|
before I do a complete review, @Fristender, do you have a strong reason that web-standalone can't just (at least initially) just import web.js and reuse exported shaders and etc? I don't mind the refactor and I know you already put work into this, but I'd generally prefer we do invasive changes as progressively as possible. In this case, separate the |
|
FWIW I can review it in this state but I will be slow. It'll be faster if you split up your changes into more well-scoped PRs that make diffing as usable as possible. I know you already split it once, but it's valuable to do these things in chunks we can reason about. |
The idea is to preserve
I am okay with this, working on this now. Also, do you guys have some tests that verify every feature works? Doesn't have to be code, could be a series of steps and the expected behavior. That way I can verify the refactor doesn't have regressions to save time. |
@david-vanderson can you comment? My understanding is there are only some zig tests and the demo window. It would be cool @david-vanderson if we had a pixel comparison threshold test for several backends, but I'm sure there are a number of problems with that. Graphl does have something like that with a screen shot diff test gh action. I should open it up |
That's correct. I've been hesitant to add too many tests given the high rate of feature changes, but that does seem to be slowing down, so maybe we are transitioning to a more test-friendly phase. Usually I click through the demo as a smoke test. For the web, I do it both on a PC and then on a phone (more limited). But I can totally do that - if you two can get the code in a shape that looks good I can do manual regression testing.
We tried that at the very beginning of the testing infrastructure but couldn't figure out how to make it work. I still think there is some kind of way here - like limited widget-tree hashing for normal tests, and specialized pixel comparison tests for low level stuff. But unsure how to proceed. |
This refactor is the prerequisite for #962
Moves all code shared between the in-page backend (
web.js) and the upcoming Worker backend into a new ES moduleweb-common.js(~1060 lines): shaders,WebRendererbase class (GL setup, textures, render geometry, import-object builder), and event helpers (encodeModifiers,touchIndex,WheelHandler,HiddenInputManager,getTouchCoords,dvui_fetch).web.jsshrinks from ~1300 to ~620 lines and now just holds the app-specificDvuisubclass.Review guide
web-common.js:1-95— shader sources +dvui_fetch, moved verbatim; check no drift vs. oldweb.js.web-common.js:96-150— string/UTF helpers andencodeModifiers/touchIndex.web-common.js:400-550—WebRendererclass;buildImports()— verify the import list is the union both backends need.web-common.js:1050-1085— default no-op host functions;web.jsoverrides the ones it implements.web.js:5— imports from./web-common.js;web.js:58—class Dvui extends WebRenderer. Everything below should be override-only; confirm no duplicated logic remains.build.zig:1641-1642—web-common.jsadded to install set forweb-test/web-app.zig build web-appstill serves and renders identically (pure refactor, no behavior change intended).