[14:02:45] Amir1: is it known that miss_rejected is elevated 1000x since this month? https://grafana.wikimedia.org/d/000000106/parser-cache?viewPanel=8&orgId=1&from=now-30d&to=now [14:02:47] I [14:02:59] I'm guessing maybe an intentional hook that is invalidating something old that we're transitioning? [14:03:03] videojs? [14:03:18] (click miss_Rejected in legend to see) [14:03:33] videojs patch went live recently [14:04:05] I think it's because some code paths during edit check the PC [14:06:02] It's been up for 10 days, but let's give it another week then to see if it comes back down [14:07:00] https://gerrit.wikimedia.org/g/mediawiki/extensions/GraphViz/+/bc0259dc4e909a9a5932b01fc1a757ce75d9858f/includes/GraphViz.php#343 ? [14:07:19] hmm https://gerrit.wikimedia.org/g/mediawiki/extensions/Wikibase/+/a82f669743865aa890b061cb9215819ea613cbf2/repo/includes/RepoHooks.php#1043 [14:07:27] I will check [14:07:46] right now, I want to get the fix for duplicate parses out [14:08:18] T301310 [14:08:18] T301310: CommonsMetadata extension is triggering a duplicate parse in commons - https://phabricator.wikimedia.org/T301310 [14:09:52] https://gerrit.wikimedia.org/r/c/mediawiki/core/+/761993 [20:14:18] TimStarling: Im running it as NODE_OPTIONS='--enable-source-maps' node --enable-source-maps tests/data/sourcemap/error.min.js [20:14:25] (tried both ways of setting options, ignore that) [20:14:32] it seems to not add the lines to the trace [20:14:50] For comparison, https://github.com/qunitjs/qunit/blob/main/test/cli/fixtures/sourcemap/source.min.js does do it [20:15:10] I thought maybe node wants the source names , but I noticed you did set names in sources:[] already, so that can't be it [20:15:30] and besides, it'll fallback to reporting it under the original name afaik if there's only only 1 (as Firefox seems to do) [21:57:53] try removing the XSSI prefix [21:58:31] the spec actually says that the )]} prefix should only be used over HTTP, so theoretically it's invalid if it's in a file [21:59:06] and note that I had to manually strip it in my mozilla test, the mozilla library is documented as taking a JSON string as input [21:59:50] so maybe it shouldn't be in the minify library, maybe it should just be added by RL if necessary [22:00:09] I just thought it was safer to put it in the library [22:04:08] TimStarling: ah, interesting. okay that did it [22:04:46] TimStarling: I'm now noticing it is implying an odd "null" object in the mapped traces: https://gerrit.wikimedia.org/g/mediawiki/libs/Minify/+/refs/changes/13/762513/2/tests/data/sourcemap/README.md [22:04:58] `at foo (error.min.js:1:35)` maps to `at null.foo (error.js:3:9)` [22:05:50] hmm, we can add a names array, but it will bloat the map [22:06:05] I thought it would figure out what to do without a name but maybe it has only been tested with the closure compiler [22:06:50] The next thing I'll do tomorrow is see how we can combine multiple of these maps. We'd need something that can concatenate multiple individually mapped minified chunks with some boilerplate in-between chunks e.g. concat( "mw.loader.implement('foo', ", mapped x.js, ");mw.loader.implement('bar');, and then we'd presumalby have to change all such operations in RL to utilise this [22:07:34] we don't change names, so it seems at glance that it shoulnd' be needed, but I havent' checked the spec on whether it requires it for that [22:08:04] the spec does not require names [22:10:40] as for concatenation, did you have a preference between the three options in my phab comment? "The minifier could provide an incremental mode, allowing you to add chunks to a state object, or it could provide a batch mode, with an array of sources, or RL could combine single-file maps using an index map." [22:11:58] the delta encoding annoyingly continues between files, so you need to place a large negative line offset to move back to the start of the source file, even though it knows you've switched source files [22:12:20] that's why they can't be trivially concatenated [22:28:20] I read it but didn't think about it enough until now, that this affects what we offer within the Minify lib indeed. [22:30:18] So a batch mode wouldn't work well I think as that make the coordination quite differnet and a bit harder to integrate. We also have literals between most chunks that we'll need to attribute to something (although if I understand correctly they would not have to per-se, in some cases they can be lost, is that right? So depending on how meaningful the boilerplate is we can eiher make it dissapear, or attribute it to something generic) [22:33:45] So 1) incremental seems interesting but I need to check more thoroughly whether that's compatible with all the various things we need to do, it seems the most attractive but also the most different, and approach 3) remap from index files, would that mean I feed back the result of (minified, map) and have the library concatenate/remap that with glue chunks? E.g. concat( [min,remap], "literal", [min,remap] ) or some such? [22:54:01] if there's chunks of output that are not accounted for in the input, you would have to notify the state object of the change in output position [22:54:18] just to make the delta encoding be correct [22:56:09] note that the source map only contains position mappings, not range mappings [22:56:35] there is a kind of null segment which only references the output but not the input, it's unclear what it is meant to be used for [22:56:50] it's really a pretty badly written spec document as these things go [22:58:19] as for 3, yes it would be possible to concatenate maps like that, but I was thinking of an "index map" https://sourcemaps.info/spec.html#h.535es3xeprgt [22:59:52] the incremental state object could just be the SourceMapGenerator class in the existing patch, I can just remove @internal and expose it somehow [23:02:07] the mozilla library uses a bisection search to find a source position corresponding to a given output position, so you could probably use those output-only segments to suppress mapping in a range