[00:17:54] TimStarling: Right, that's classic globalEval. [00:18:09] Full circle. [01:24:59] how much do you care about errors being at the start, on a scale from 1 to 10? [01:25:36] Errors at the end -- one second of work. Errors at the start -- one hour of work and 50 lines of code. [01:26:44] also even after an hour of work there is a fair chance the errors will corrupt the source map [01:27:04] since the exact text of the errors would have to be consistent for a given version [01:29:21] TimStarling: For JS, we have the mw.log.warn() call, right? That'd be fine to have at the bottom. The comment is nice to have, but I don't feel strongly about it. We have good error logging tools nowadays. [01:30:33] mw.log.error* [01:30:46] Thanks, that will make things simpler. Yes there is a log call except for only=styles [01:31:52] Ah it uses console.error, grep failed. [01:31:57] makes sense, so it works on startup too [01:32:50] If it's trivial to do, keeping them at the top for only=styles could be nice for discovery. [01:33:00] no sweat either way :) [01:33:14] CSS doesn't have a source map, so it's easy enough for now [01:34:06] you can append to the generated source without disturbing the source map, but prepending is a couple of orders of magnitude more complex [01:34:52] so this means my minify branch is ready for review [01:35:07] yeah, no problem. [01:36:32] TimStarling: I'm slightly confused why though. My last understanding was that you still minify/cache each module chunk separately and put them together at the last minute in a high-level place where each chunk is combined in 1 large sourcemap. I would have guessed (wrongly) that adding an item there before we do that would make little difference. I'm guessing we build the high-level object as we go? [01:37:32] the data model in IndexMap currently doesn't allow prepending, but it would be easy enough to change it so that it does allow it (this accounts for most of my one hour estimate) [01:37:52] the ugly thing is that errors are only finalized in RL::respond() whereas the IndexMap is finished in makeModuleResponse() [01:38:06] also makeModuleResponse() is public and called from various places [01:38:43] so the most likely plan would have been to make makeModuleResponse() be a b/c wrapper around a new private method that returns an IndexMap [01:39:22] for single module requests I'm not building an index map, so for errors in that case you'd have to make a new IndexMap late in the process [01:39:35] ack, I see the gap now. makes sense, not worth it indeed. [02:03:23] TimStarling: I'm probably using it wrong, but trying out based on reading the API and test case examples https://gerrit.wikimedia.org/r/c/mediawiki/libs/Minify/+/947494/1 [02:03:36] Firefox attributes the lines in indexmap.html all to foo.js for me [02:04:39] nvm, bad for loop. works now [02:04:51] Still could use a review to make sure I've used it in the way you intended [02:07:02] I can upload the WIP patch if you like, then you can try the full stack [02:08:34] I'm reviewing [02:14:06] all looks fine, but maybe tests/data/sourcemap/README.md needs to be updated? [02:16:50] ack [02:18:05] TimStarling: btw, one thing I do notice in both Chrome and Firefox is that virtual files (i.e. those not exposed or fake placeholder names) results in errors in devtools when clicked on. I thought maybe this is unavoidable, but looking at source maps for OOUI for example, it seems to work there. For example, mw.loader.load('https://unpkg.com/oojs-ui@0.47.5/dist/oojs-ui-apex.js'); on a local page, allows one to explore the internal src/ [02:18:05] files despite them not being published at those URLs. [02:18:53] https://usercontent.irccloud-cdn.com/file/QwZ3xAc1/Screenshot%202023-08-10%20at%2003.16.40.png [02:19:37] whereas on combine.html and indexmap.html, neither allows browsing those chunks [02:20:56] I'm guessing this relates to $needSourcesContent not being true. [02:21:31] are you saying the file is neither bundled nor a valid URL? [02:21:44] I gotta go now, but looks like you already accounted for this and probably will or already have in MW patches, but I didn't trigger it right in the standalone demo. [02:21:47] in MW the file contents are all bundled [02:22:23] yeah, I see sourcesContent in the OOUI map, and you implemmnted this already in Minify, but I'm not triggering it it seems. Maybe I faked it too well. [02:23:03] I should have a fully working MW commit up by the end of my day