[21:14:56] test [21:15:24] brendan_campbell, it works, congrats :) [21:17:32] Thanks Dysklyver! There may be something weird going on with irccloud right now [21:59:04] who is here for the RFC meeting re npm/yarn? [21:59:20] * tgr is lurking [21:59:24] hi [21:59:38] Krinkle? [22:00:06] Aye, been caught up in things [22:00:10] Yep, I am here. [22:00:34] #startmeeting RFC meeting [22:00:34] Meeting started Wed Jan 31 22:00:34 2018 UTC and is due to finish in 60 minutes. The chair is TimStarling. Information about MeetBot at http://wiki.debian.org/MeetBot. [22:00:34] Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. [22:00:35] The meeting name has been set to 'rfc_meeting' [22:00:35] Meeting started Wed Jan 31 22:00:34 2018 UTC and is due to finish in 60 minutes. The chair is TimStarling. Information about MeetBot at http://wiki.debian.org/MeetBot. [22:00:35] Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. [22:00:35] The meeting name has been set to 'rfc_meeting' [22:00:54] #topic RFC: MediaWiki support for Composer equivalent for JavaScript packages [22:01:03] #info https://phabricator.wikimedia.org/T107561 [22:01:04] hello [22:01:24] \o [22:01:32] Hey. [22:01:53] * Dysklyver is just watching [22:02:11] Volker_E ^ [22:02:13] hey! [22:02:15] I'm hoping today will help ourselves understand the exact nature of the problem. [22:03:00] looks like there was a lot of discussion on the ticket since yesterday [22:03:11] The RFC is getting older, and various ideas have been proposed from time to time. However, unless we define exactly what problems we want to solve, and what the requirements should be, it's difficult to move forward. [22:04:06] I outlined the main problems I want to see solved by this in https://phabricator.wikimedia.org/T107561#3935220 [22:04:11] I propose to split this up into three sub topics, 1. problems, 2. requirements/preferences for what the outcome will (not) be capable of, 3. possible solutions. I suspect we won't have time for #3, but with #1 and #2 behind us, we can do that offline. [22:05:08] currently we have a number of bundled JS libraries, correct? and the problem is that we want them to be automatically managed? [22:05:24] Let's start with problems we currently face. Try to be brief. [22:05:53] TimStarling: Right, I believe that's a relatively small part of it, but definitely one. [22:06:22] on a comment on the ticket you mentioned replacing update-oojs.sh [22:06:37] #info Problem: We manually download upstream libs and commit to Git. We'd like to automate this to some extent. Whether outside Git, or within git with a script. [22:06:39] Right now updating jQuery in core requires someone to copy and paste into a file and commit [22:06:56] Well, copy from npm, but yes. [22:07:03] (Speaking as the person that did the last upgrade.) [22:07:11] #info Problem: Updating / tracking out of date libraries is ad hoc at best, non-existent for most. [22:07:14] As I wrote on the ticket there's also a need to share JS code across extensions, without having to put it in core, e.g. Minerva depends on code in MobileFrontend, but shouldnt need to [22:07:23] Yeah, so for example, our composer approach for PHP does not mean we don't commit to Git (at least for prod, we still commit to mediawiki/vendor), but it is scripted with 'composer update', and for local dev can be avoided entirely. [22:07:46] legoktm: Right, we don't currently have a manifest saying which version we use. One has to check the file headers or commit log. [22:08:02] And we quite often have patches on "libraries". [22:08:10] #info Problem (cont'd): Lib versions are inferred based on file contents and/or commit log, not a self-documenting manifest of sort. [22:08:15] #info Problem: No automated security scanning for JavaScript dependencies [22:08:17] Which you can only see through git or very careful reading. [22:09:05] Don't we also have a problem of discoverability of libraries that have seen a good amount of work by Foundation product teams for our needs, eslint with grunt-eslint and eslint-config-wikimedia or stylelint with grunt-stylelint and stylelint-config-wikimedia come to my mind… [22:09:05] #Info Requirement: Consider ease of forking/patching of current approach. [22:09:11] #info Problem: JS package management is MW specific instead of canonical JS. [22:09:32] niedzielski: what do you mean by "canonical JS"? [22:09:33] Volker_E: This is for production libraries, not dev libraries, I think. [22:09:34] niedzielski: Can you elaborate on this? [22:09:43] I believe Wikibase/Wikidata has a bunch of JS libraries too - not sure how they align with those in core... [22:09:52] niedzielski: do you mean based on ResourceLoader rather than npm ? [22:10:46] The scope of this RFC does include extensions, in so far that the ease of adding third-party JS libs applies to extensions as well. Which is one of the requirements we'll have, we can't just have a script for core, it needs to scale and potentially account for de-duplication accross repos as well. Currently composer does this with local-merge plugin. [22:10:55] #info Problem: Enabling extension and skin authors to depend on npm libraries without duplication [22:10:59] installing a package requires deep knowledge about how MW loads resources instead of well, "just working" like all the other JavaScript repos [22:11:09] #info Requirement: Support extensions as well (de-duplicate, still easy to fetch all at once for installed extensions). [22:11:53] niedzielski: are you looking for automatic RL registration? or npm actually replacing RL? [22:12:06] niedzielski: Right, but even if we have support for package.json and use 'npm install' to download them, MediaWiki JS still runs in the browser, not Node.js. Which means we still need to specify somewhere which script files to send to the browser. [22:12:11] SMalyshev: re Wikibase js libs we currently avoid commit these to wikibase.git by having them included there as git submodules. Which is pretty close to commiting libs to repo [22:12:45] joakino: "wihtout duplication", are you referring to an extension wanting to use a js lib that is not in core, and then extension A and extension B both need to conditionally register their copy? [22:12:49] Or something else? [22:12:52] niedzielski: or do you mean require()? [22:13:30] wasn't there another RFC for require()? [22:13:38] TimStarling: as a developer, it would be ideal for `npm install` to work as expected without having to manually modify ResourceLoader. i think discussion around replacing RL are out of scope [22:15:38] leszek_wmde3000: exactly. which probably has the same issues, plus potential conflicts with core ones... [22:15:50] my preference would be best to use standard ES6 imports so that MW JavaScript read like JavaScript elsewhere [22:16:10] Krinkle: yes, imagine extension A uses redux from npm, and extension B wants to use it too, then they either register it as a module always risking duplication, or conditionally check if others included it, risking version mismatches, etc. Those kind of problems, libraries that don't necessarily belong in core [22:16:15] that does sound a bit like require() [22:16:37] TimStarling: Just to clarify, 'npm' and RL are not comparable as alternatives. npmjs is not a delivery system for client-side, it's comparable to packagist.org and the 'composer' CLI tool. npmjs does not have an autoloader. In Node.js there is also no autoloader equivalant, given that in JS references are always by relative file path, never by class name or some such. The probelm however is that in Node.js (like PHP) you can load [22:16:37] things after the fact, and synchronously. Whereas on the web, this isn't the case, so all files need to be known in advance. [22:16:57] joakino: Thanks, yeah, that makes sense. [22:17:34] Krinkle: understood, and I guess I'm unclear on niedzielski's "as expected" [22:17:40] #info Problem: Currently, MW extensions using js libs need to conditionally register their module based on whether another extension has already registered it. This means version-needs are not honoured or even checked. [22:18:11] The ability to use require() with a module name, and the ability to export an interface from a module, without using a global variable, is already present in MediaWiki. [22:18:43] We essentially implement the same CommonJS interface that require.js, AMD, WebPack and Node.js implement. [22:18:58] e.g. require('oojs') works. [22:19:03] in module scope [22:19:21] Or, from the browser console, as mw.loader.require('oojs') [22:19:25] (for debug purposes) [22:19:34] TimStarling: sorry, what I mean is that `npm install ` makes the dependency (at the version specified) available in the current extension using the `import` keyword with no other changes (except what NPM does to package*.json) [22:19:58] in the matter of common.js and require, it seems important to note that the JS standard has approved ES modules and they are already shipping in browsers, so it is probably wise to not invest in Common.js and if anything invest in EcmaScript modules (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import) [22:20:03] import could theoretically be statically inferrable, right? unlike require()? [22:20:08] joakino: agreed [22:20:56] i.e. we would have RL parse JS files for import statements, and deliver the necessary modules to the client [22:21:00] For the record, 'import' is essentially an ECMA-standardised version of require() (part of ES6). Implemented in browsers by pausing execution and sychronously downloading said file. [22:21:40] so is this out of scope? [22:21:58] This is not a scalable or performant model for the web, it's mostly for Node.js. Some transpilers support it right now, but only as a way of pretending what really happens. In reality it just loads all the 'import' references ahead of time, and replaces your 'import' line with a direct reference. In other words, anyone using ES6 'import' on the web, isn't really using it. [22:22:09] tbh we only use `require` in RL because doing `import` would require a transpile step [22:22:16] yes TimStarling, it is a feature of the module system, fully statically analyzable (except for dynamic imports, which are the standard version of mw.loader, https://developers.google.com/web/updates/2017/11/dynamic-import) [22:23:05] those *can* be analyzed but *may not be* analyzable [22:24:06] Okay, before anyone says "webpack" (or another js library that parses JS files recursively to find references, bundle them, and replace references) - MediaWiki delivers content from PHP. [22:24:28] seems like a separable concern, we have enough to talk about in this RFC without getting into transpilers [22:24:29] Is de-duplication a problem we have to solve now? I understand that it is a real problem, but can we use npm to keep track of libraries and then address deduplication in a follow-up RfC? [22:25:14] legoktm: I wouldn't mind deferring it, but I think 'using npm' is sufficiently complex, that if we do, it likely solves that automatically. [22:26:01] legoktm told me previously that he wants to avoid a non-standard merge plugin like we use for composer [22:26:06] at least to start with [22:26:17] Unless 'using npm' means, having a shell script like we do for 'oojs' that downloads it on command, or even one that reads a package.json-like file, and automates it. We'd still need to know which files to move from /tmp to /resources and list them in the module definition, in the current system. [22:26:43] Krinkle: in https://phabricator.wikimedia.org/T107561#3935220 I outlined a proposal to use npm that is mostly a drop-in replacement for what we do right now [22:26:44] for non-core (extensions, skins) definitions of dependencies the duplication matters. If an extension could specify something in ResourceModules like 'npm/redux' with the version, and we could get warnings in logs for example if 2 incompatible versions of the same package are found in the whole installation, that could be good enough [22:27:03] most npm libraries expose umd modules that attach to window which we could use from extensions/skins/core [22:27:05] legoktm: If we have dupicated libraries i think it's good that we have that problem to solve. It feels like that would be a social problem e.g. JavaScript console would complain "Hey! Two libraries with the same name but different versions were registered! Oh no!" and then team A and team B would need to talk about getting in sync with their version numbers. [22:27:10] Also, the plain version of 'using npm' described above, would not support packages that have dependencies. [22:28:18] It feels like what we are asking for is mapping compiled assets inside npm modules to ResourceLoader [22:28:40] in such a way that any extension can register them and RL will deal/let us know about version clashes [22:29:16] So it would be limited to packages that are published to npm as a single js file? [22:29:34] yes. seems like a good first step Krinkle [22:29:50] Are most of our dependencies published like that? I was hoping so. [22:29:52] isn't everything on npm has dependencies? each time I use some package it brings in like 20 others... [22:30:08] For Node.js-typical packages, yes. [22:30:16] For browser code like this use case, not nearly as common. [22:30:31] ah ok, different type I guess [22:30:36] And when it does happen, it's typically hidden from publishing step (e.g. dev-only and then inlined) [22:30:52] Krinkle: that would solve including npm libraries, and versions/duplication concerns, and already work for many useful libraries for the browser [22:31:20] SMalyshev: also many popular libraries provide umd files for use as a single package, so they bundle the library for you [22:31:36] joakino: ah, cool, that hepls [22:32:02] joakino: So imagine for a second that we'd list our deps in package.json with name and version, and that (for development) we'd use 'npm install' to fetch them. We'd still list each package name in Resources.php with a path to where the file(s) are in node_modules. And we'd have to do the same for any dependencies. [22:32:13] examples of js libraries with a dist/ folder commited on npm, immutable: https://unpkg.com/immutable@3.8.2/dist/ , redux: https://unpkg.com/redux@3.7.2/dist/, and many more [22:32:23] Some concrete examples - my team is using redux and redux-thunk - it's totally possible Wikidata will soon use this and we'll want to manage that dependency on wikis which are using both our extensions [22:32:41] And for production, we'd need to store them in a git repo, at which point, the problem arises, how do we do it across extensions? Or do we commit to git in each repo, and find conflicts at run-time? [22:32:57] That would be closest to current situation, and perhaps a good first step. [22:32:59] Krinkle: conflicts at run time/deploy time seems a good first step [22:33:19] Krinkle: adding just the dist/xxx.min.js file to the git repo seems like a sensible start [22:33:53] TimStarling: We're essentially discussing something similar to how one would (in the past) automate fetching code from PEAR in a convenient way. [22:35:01] can we do our own minification? [22:35:05] thinking of https://hackernoon.com/im-harvesting-credit-card-numbers-and-passwords-from-your-site-here-s-how-9a8cb347c5b5 [22:35:13] Krinkle: I proposed just commiting to extensions repos for now, and leave conflict resolution to run time, which is the status quo [22:35:32] TimStarling: Sure, I mean that would be the default behaviour. [22:35:40] TimStarling: yeah, with jsminplus. I don't think we should be commiting minified code [22:35:44] But that doesn't prevent authors from publishing it to npm in already minified form. [22:35:52] We do not use jsminplus. [22:35:58] .. for minifying. [22:36:04] we minify in PHP [22:36:11] with JavaScriptMinifier. [22:36:16] jsminplus is way too slow. [22:36:40] my bad [22:36:55] I keep forgetting why we still have it in Git, and then I remember, but it's not a very good reason. [22:37:30] (jsminplus has better error messages when code is invalid, so we use it only for validating .js gadgets and user scripts, but still minify with the other one) [22:37:48] Eww. [22:38:17] #info As first step, we might want to keep things in Git like now. But focus on automating the fetching of things from npm, and focus on declaring the package names/versions in a machine-readable and standard format (eg. package.json) [22:39:07] here's my proof of concept of that implementation: https://gerrit.wikimedia.org/r/#/c/407055/ [22:39:24] it still needs some fine tuning so we aren't commiting the entire jquery source tree in addition to the combined build file [22:39:26] So when extension.json gets read, package.json also gets read and extension.json mentions which modules in package.json should be registered in RL? [22:39:38] I imagine committing node_modules will not be pleasant. I already have big problems with a project that commits vendor/ to git, but only for non-dev dependencies, very messy. I suppose if we script the fetching of dependencies, we can keep them in /resources/ like we do now. [22:39:47] jdlrobson: no, you'd still manually specify module definitions in extension.json [22:39:51] Krinkle: Yeah. [22:40:06] Krinkle: if we put package.json in resources/ we can avoid that issue I think. [22:40:16] Hm.. I guess [22:40:43] that feels icky.. :/ [22:40:57] legoktm: in terms of extra files in the distro, npm supports a "file" attribute in package.json to white list which files to publish. Similar to how we use gitexclude for our composer exports. [22:41:06] But it seems not everyone uses them, same issue as with vendor really. [22:41:07] for committing packages, it'd be probably better to just commit the dist files going to be used, so instead of commiting the full node_modules/jquery we could commit just resources/node_modules/jquery/dist/jquery.js [22:41:08] legoktm: with respect to extension.json I guess what I'm saying is automate that e.g. "redux@{version}": { class: "ResourceLoaderNPMModule" } [22:41:32] jdlrobson: seems reasonable, but I think that can be discussed/implemented independently? [22:41:44] legoktm: yeh np. [22:41:49] joakino: yeah, I'm leaning towards that. just commit README, LICENSE, and dist/ [22:41:49] jdlrobson: One issue there is that there is no standard for which files are needed. There is a standard for "main" in package.json, but from there it can require() relatively anything else. [22:42:02] So we really do need a "scripts" list still, unless we try to scan for all require references. [22:42:29] 👍, note that package names can contain @ if they are scoped, see https://docs.npmjs.com/misc/scope [22:42:34] legoktm: We can also upstream improvements to package.json for distributing fewer files [22:42:42] Aye, '@' is invalid in RL module names. [22:42:47] Good point [22:42:47] legoktm: If the packagers have done their job correctly, that's what npm install should output. :-) [22:42:55] Krinkle: definitely :) [22:43:14] #info for committing packages, it'd be probably better to just commit the dist files going to be used, so instead of commiting the full node_modules/jquery we could commit just resources/node_modules/jquery/dist/jquery.js [22:43:18] #info Problem problem: Npm module names support namespaces with an '@', which RL has already reserved. Think about... [22:43:22] #info legoktm: We can also upstream improvements to package.json for distributing fewer files [22:43:37] so all NPM dependencies are loaded regardless of which module is requested? [22:43:40] #info joakino: yeah, I'm leaning towards that. just commit README, LICENSE, and dist/ [22:43:41] er RL module that is [22:43:43] Krinkle: I guess the version could live in the theoretical ResourceLoaderNPMModule - which gets populated with whatevers in package.json - would make it easier to detect (but i think im getting into implementation details here so will stop as it's out of scope of your original goal of just identifying the problem) [22:43:46] neilpquinn: registered != loaded. [22:43:55] niedzielski: all npm dependencies are commited to git / registered, but not loaded unless needed [22:44:03] loading will still be with addModules() or mw.loader, nothing in this RFC should change that. [22:44:09] 👍 [22:44:47] Krinkle: makes sense having the scripts to specify which files to load, there is no convention or anything in package.json [22:45:29] joakino: that means however that after you run 'npm install', you'll spend a lot of work manually scanning node_modules and grepping files until you have all the references needed. [22:45:52] Right now that is somewhat avoided by using the official downloads from library websites instead, which are bundled into a single file already. [22:46:04] (may be missing some context) when we say committed to git, we're talking about mediawiki/ext repo or something like mediawiki-vendor repo? [22:46:21] Krinkle: shouldn't `npm install` have that single file bundle? [22:46:26] When we download from npm, those same packages typically don't pre-bundle that way. And that's good because it'd be negative for Node.js to bundle things that way. [22:46:32] SMalyshev: mediawiki/core and mediawiki/extensions/Foo for now [22:46:37] right, manually adding the files to git from the node_modules folder [22:46:49] legoktm: Nope, jquery is an exception to the rule because its src/ is not actually runnable, it's just for maintaining files in smaller chunks. [22:47:19] I would not expect npm to provide a single file for a package. That would also not be desired generally. [22:47:45] so we need to do our own bundling? [22:47:50] And we support require(), and soon for multiple files as well (separate RFC underway), but that does mean we need to list each file needed. [22:47:54] as a convention, popular browser libraries put the single file bundle under dist in the npm package [22:48:03] No, bundling and require() emulation is all in RL already. [22:48:15] hmm having all 3rd party libs in mediawiki/core does feel kinda icky [22:48:21] including concatenating and minifying in a way that preserves information about file boundaries (see separate RFC) [22:48:22] you can check libraries you would use without downloading with this npm cdn https://unpkg.com/redux/ [22:48:25] check the contents [22:48:40] sep rfc is T133462 [22:48:40] T133462: Provide standard-compatible way to load multi-file packages (not plain concatenation) - https://phabricator.wikimedia.org/T133462 [22:49:03] SMalyshev: I don't think we're proposing to have them in all in core. [22:49:17] for security it makes sense to me to have unbundled, unminified JS in git, that way we can do integrity checks in jenkins [22:49:39] ah good i just read the backlog and there was talk about committing to git, so I wondered... [22:49:40] Yep, the files come unbundled and unminified from npm. [22:50:34] SMalyshev: Yeah, to git, but not all in one place. Same as today basically as first step. But with registration semi-automated. And conflicts will result in run-time errors. So we'd basically come up with a way to allow two extensions to register the same third party lib, as long as its version is the same (or compatible). [22:51:05] Whereas right now we hard error on any name conflict, which leads to the current mess where extensions have to conditionally register their third-party module from a hook. [22:52:35] seems like nobody has disagreed with the basic plan for a while, do we have consensus on it? [22:52:42] ah so we're not moving them around just recording them properly for starters/ [22:52:55] TimStarling: I think so. [22:53:05] * legoktm has to run to class now, ttyl :) [22:53:09] this is a good first step. Thank you for this good conversation! [22:53:20] I suspect that people might not yet realise the added inconvience of manually listing out all files from the third-party lib. [22:53:21] agreed [22:53:29] next step how do we go about implementing this? [22:53:32] joakino: So for redux, we should list out hte files in /lib, not the one in /dist. [22:53:34] Krinkle: Currently we have to do that in Resources.php… [22:53:36] sounds like legoktm has a starting point? [22:53:45] yeah, legoktm is writing code already [22:54:01] Krinkle: why not just include https://unpkg.com/redux@3.7.2/dist/redux.js ? [22:54:47] there is an unminified file that is readable [22:54:52] joakino: Well, it seems that dist is specifically intended for users without a package manager. It also bundlees a copy of require.js which we don't need, and logic for exposing a global variable, which we don't want. [22:55:09] joakino: Yeah, it is readable, but calling require() will lead to lib/index.js in Node, and in Common.js [22:55:27] the dist/ file is presumably there for ease of inclusion from raw