loader, docs, test: named exports from commonjs modules #16675

devsnek · 2017-11-02T00:03:38Z

I know a lot of discussion went into the original decision on how mjs would handle cjs modules but after using the system for a while, and talking with a lot of other people in the community, it just seems like the expected behavior and the wanted behavior is to export named based on the keys. This PR implements that in what is hopefully a performant enough solution, although that shouldn't be too much of a problem since this code only runs during initial module loading.

This implementation remains safe with regard to named exports that are also reserved keywords such as class or delete.

EDIT: many changes to original impl, definitely read comments and file diff

Refs: nodejs/node-eps#57
Refs: nodejs/abi-stable-node#256 (comment)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

loader

I know a lot of discussion went into the original decision on how mjs would handle cjs modules but after using the system for a while, and talking with a lot of other people in the community, it just seems like the expected behavior and the wanted behavior is to export named based on the keys. This PR implements that in what is hopefully a performant enough solution, although that shouldn't be too much of a problem since this code only runs during initial module loading. This implementation remains safe with regard to named exports that are also reserved keywords such as `class` or `delete`. Refs: nodejs/node-eps#57

addaleax · 2017-11-02T00:30:30Z

I think I am in favor of the general approach, but cc @nodejs/tsc because if we decide to go with this we’re likely stuck with it.

This implementation remains safe with regard to named exports that are also reserved keywords such as class or delete.

Can you add a test for that?

mcollina · 2017-11-02T08:57:35Z

doc/api/esm.md

-representing the value of `module.exports` at the time they finished evaluating.
+When loaded via `import` these modules will provide a `default` export
+representing the value of `module.exports` at the time they finished evaluating,
+and named exports for each key of `module.exports`.


I would add a sentence explaining what will not be supported, i.e. modifying module.exports with an asynchronous operations will not update that property in ESM. It seems an edge case, but I do not see a way to provide a good error message for it we should be careful in how we document.

mcollina

LGTM with some nites

mcollina · 2017-11-02T08:58:35Z

test/es-module/test-esm-namespace.mjs

-assert.deepStrictEqual(Object.keys(fs), ['default']);
+assert(fs);
+assert(fs.readFile);
+assert(readFile);


can you test that readFile is the same as fs.readFile?

targos · 2017-11-02T11:01:12Z

/cc @guybedford

I think this fixes #16474 by moving the execution phase of CJS modules. Could that cause real issues?

A very very large portion of the js community (babel/typescript) use `__esModule` to define an es module in CJS.

guybedford · 2017-11-02T16:32:58Z

Update: most of these points are no longer relevant when restricting named exports extraction (Object.keys(module.exports)) to cases where exports.__esModule is defined

I'm glad this can be properly considered in its technical context now. But there are serious concerns here which need to be carefully discussed, so I wouldn't advise rushing this by any means:

Performance is not a trivial consideration - iterating all exports for 100s of modules may have a significant slow down so it would be nice to get some actual numbers to this on real application cases before any merging.
Of course execution timing and exports mutations are the known concerns as discussed much before. I'm surprised the esm-snapshot.mjs test isn't failing, as that is supposed to be a test for these kinds of cases.
I'd be very concerned about the triggering of getters. For example ~~of a breaking case, this would fail loading~~ chalk, which has getters on its prototype, which is then exported as an instance on module.exports (https://github.com/chalk/chalk/blob/e2a4aa427568ff1c5d649739c4d1f8319cf0d072/index.js#L59). This code loading chalk would then ~~fail as it would~~ trigger every getter on module.exports ~~without this being bound correctly~~. Handling these kinds of issues make this approach much harder to get right. Correction: the this binding would still be correct, I can't recall what the failure might have been here, which may have been more specific to a SystemJS use case.
Finally, there's also an important case to consider when one of the keys is itself a default. Firstly, this case needs to be handled in the code above. Then secondly, if a module wants to be transparently replaced with its ES module, then there are breaking cases to be found here as well (if I remember correctly react-router or some similarly core react lib is a good example of a breaking case here). Personally I think that that sort of exception might be ok, but it needs to be a carefully considered decision.

Getter triggering and performance as above are probably my biggest concerns here, for which there don't seem to be easy paths forward.

devsnek · 2017-11-02T16:53:11Z

with my last commit here i changed the implementation a bit (see the commit message) so @mcollina you might want to revoke your approval until you re-review it.

to respond to @guybedford i suppose it wouldn't be too difficult to use getOwnPropertyDescriptor although that would raise serious performance concerns for me. this could also be fixed by my latest commit and chalk choosing to not use __esModule, if __esModule doesn't get too much backlash

reasoning behind __esModule which at first may appear to be a bit of a hack:
This __esModule property is already in use by somewhere between thousands and millions (i'm thinking the latter) of modules and projects. This would also allow an easy way for lib authors who need compat with their library from old node versions all the way up to latest with a super easy upgrade path (check how you want to use exports.default then add __esModule), and a babel'd or typescript compiled code bundle would work out of the box with node's import.

the only bad thing about this is that it might make people get weirded out when then try to go to browser with the expectation that it will work the same way, but i'm hopeful that with good docs and explanation we can not have to worry about this.

mcollina

Thanks for heads up @devsnek, yes, there is something I would like to change.

mcollina · 2017-11-02T17:06:23Z

lib/internal/loader/ModuleRequest.js

+  const CJSModule = require('module');
+  const pathname = internalURLModule.getPathFromURL(new URL(url));
+  const exports = CJSModule._load(pathname);
+  const es = !!exports.__esModule;


I think we should promote the use of a Symbol instead. Adding a property might not be feasible for a lot of modules. We should also support __esModule for backward compat with babel.

thats a fantastic idea, would attaching it to the module module, perhaps as Module.esModule be good?

Perhaps something in the name to indicate it is a symbol for CommonJS interop cases? esModuleInterop?

I'm fine with making it as long as we want. I would use Symbol.for() so it can be backward-compatibile with older versions of node that do not have the symbol, plus other tools that might want to process it.

guybedford · 2017-11-02T17:36:39Z

@devsnek nice, __esModule as an opt-in and a symbol seems like it could be a very sensible option here, and chalk doesn't seem to use it currently. It also handles the dealing with the default collision case quite nicely as well. For what it's worth, I created the __esModule flag pattern (babel/babel#95) so am somewhat biased in supporting it :P

recommends @@esmodule over __esModule

mcollina · 2017-11-02T17:55:35Z

lib/internal/loader/ModuleRequest.js

@@ -19,6 +19,8 @@ const search = require('internal/loader/search');
 const asyncReadFile = require('util').promisify(require('fs').readFile);
 const debug = require('util').debuglog('esm');

+const esModuleSymbol = exports.esModuleSymbol = Symbol('esModule');


I would prefer Symbol.for('esModuleInterop') so that an import of 'module' is not required.

that was my first approach but it failed the tests. i speculate that symbols aren't shared between v8 contexts or something, if you have a solution please lemme know

> vm.runInNewContext('Symbol.for("foo")') === Symbol.for('foo') true

I think this should work?

mcollina · 2017-11-02T17:57:05Z

lib/internal/loader/ModuleRequest.js

+  const keys = es ? Object.keys(exports) : ['default'];
+  return createDynamicModule(keys, url, (reflect) => {
+    if (es) {
+      for (const key of keys)


There might be some edge cases where module.exports is a proxy, can you please check if that would cause problems?

I think we should use a for(;;) loop here. They are faster.

addaleax · 2017-11-02T17:56:55Z

doc/api/esm.md

+CommonJS modules, when imported, will be handled in one of two ways. By default
+they will provide a single `default` export representing the value of
+`module.exports` at the time they finish evaluating. However, they may also
+provide `@@esModule` or `__esModule` to use named exports, representing each


What’s the motivation for keeping __esModule when a symbol is available? You can always import { esModule } from 'module';, right?

Or, just a thought: What if this was an alternative to module.exports, e.g. module.esmExports? We could check for the presence of that property when the script finished.

The upsides of this would be that you don’t need a symbol, and you don’t need to modify the actual exports object

mcollina · 2017-11-02T17:59:12Z

Considering that we are supporting __esModule, can we add a module transpiled by babel to cjs
(more than one version of it if it had major output changes) to the tests?

addaleax · 2017-11-02T18:02:00Z

lib/internal/loader/ModuleRequest.js

+  const pathname = internalURLModule.getPathFromURL(new URL(url));
+  const exports = CJSModule._load(pathname);
+  const es = Boolean(exports[esModuleSymbol] || exports.__esModule);
+  const keys = es ? Object.keys(exports) : ['default'];


Would Object.getOwnPropertyNames(exports) make more sense?

i chose to use Object.keys so that it only exports enumerable properties. (it says so in the esm doc)

addaleax · 2017-11-02T18:05:40Z

If I understand correctly, this would currently require the imported module to opt into this mechanism? I don’t think that’s a good idea at all… if you want to opt in, you can already just go with a wrapper .mjs module.

If the primary goal is interoperability with the existing ecosystem (and for me it is), then I think this should be on by default for all CJS modules.

mcollina

LGTM

devsnek · 2017-11-02T18:41:34Z

@addaleax i'm fairly sure that enabling this behaviour by default would be kinda dangerous

@mcollina i might have to use a v8 runtime intrinsic to check if something is a proxy (IsJSProxy) if you guys are ok with baking one in bootstrap_node.js, and that might also have issues with chakra, do you guys have a binding to detect that?

addaleax · 2017-11-02T18:44:07Z

@devsnek Why would it be dangerous? I get that the mapping won’t work 1:1 but really, it should be fine for nearly everything, right?

Also, I don’t quite get what the concern with Proxies is. Why do we want different treatment?

devsnek · 2017-11-02T18:46:52Z

i think the safest thing to do by default at this point is probably just the existing behavior (sad ikr) where it attaches to default because of module.exports.default

as for proxies, trying to iterate over them could do interesting things depending on their setup like create an infinite loops

addaleax · 2017-11-02T18:52:06Z

@devsnek I don’t quite see why it would be safer or not to do that? Maybe it would help if you could explain what “safe” and “dangerous” mean here?

And sure, iterating over Proxies could do weird things (and in particular, throw exceptions) – but I wouldn’t say that’s our problem. If somebody writes a buggy Proxy, it’s broken anyway, right?

Also: Users are opting into whatever behavior we choose when they make the transition of the importing module to ESM, or writing new code as ESM. If there are problems with importing existing ecosystem code, they can report those as bugs, right?

addaleax · 2017-11-02T18:53:21Z

Like, I really liked the initial version of this PR. ;)

devsnek · 2017-11-02T18:55:29Z

i guess if the feature can be disabled [Symbol.for('esModuleInterop')]: false it could be enabled by default, but it would be cool to get more feedback on that from other people

guybedford · 2017-11-02T18:56:17Z

doc/api/esm.md

+CommonJS modules, when imported, will be handled in one of two ways. By default
+they will provide a single `default` export representing the value of
+`module.exports` at the time they finish evaluating. However, they may also
+provide `@@esModuleInterop` or `__esModule` to use named exports, representing


Perhaps clarify exactly what these properties are - "provide a @esModuleInterop or __esModule boolean export indicating that a snapshot should be taken of each enumerable key..."

sounds good, and is it @@ or @ to doc a symbol, i thought it was two

Yeah it's still two, that was a typo.

BridgeAR · 2017-11-23T22:21:20Z

Without reading everything - how can and should we progress here?

guybedford · 2017-11-24T13:47:23Z

@BridgeAR I believe @bmeck is presenting to TC39 next week an agenda item to discuss the execution ordering in the specification and also to consider locking down the spec to ensure such a feature like this would not be possible by ensuring execution ordering for other format interops. So the current status of this PR / feature is dependent on how that discussion goes.

jacksonrayhamilton · 2017-11-27T07:40:48Z

I don't think Node should provide the properties of the module.exports object as ES exports.

In Node, it's common to import modules like this:

// foobar.js, a CJS file
module.exports = {foo: 'bar', default: 'baz'};

// another CJS file
var foobar = require('./foobar');
foobar.foo // => 'bar'
foobar.default // => 'baz'

Therefore, it's important that module.exports be the ES default export, so we can do:

// foobar.js, a CJS file
module.exports = {foo: 'bar', default: 'baz'};

// an ESM file
import foobar from './foobar';
foobar.foo // => 'bar'
foobar.default // => 'baz'

Node must have agreed, since CJS is already importable in ESM like that.

Now, recall that import a from 'a' is actually syntactic sugar. The following two imports are equivalent:

import foobar from './foobar';
import {default as foobar} from './foobar';

We can visualize "the set of things exported by a module" as a map. In the case of foobar.js:

{
  default: {foo: 'bar', default: 'baz'}
}

The set of names in the "module exports map" is a different set of names than the keys of an object which is a default export. Here's why:

Say that I have an ES module like this:

export default {foo: 'bar', default: 'baz'};
export var foo = 'qux';

We can visualize that ES module's "module exports map" like this:

{
  default: {foo: 'bar', default: 'baz'},
  foo: 'qux'
}

The set of names in this ES module's "module exports map" is ['default', 'foo']. The set of names which are keys of the object which is the default export are ['foo', 'default']. The two "default" and the two "foo" are different and not equatable. When a user imports "default", they should get the object, not "baz". When a user imports "foo", they should get "qux", not "bar".

Going back to the "module exports map" for foobar.js, which looks like this:

{
  default: {foo: 'bar', default: 'baz'}
}

Given that, when we do this:

import {foo} from './foobar';

Then what should happen?

The set of names in the CJS module's "module exports map" is ['default']. The set of names which are keys of the object which is the default export are ['foo', 'default']. As established earlier, the members of the two sets are different and not equatable. When a user imports "default", they should get the object, not "baz". Following our earlier logic, when a user imports "foo", there should be an error, and they should not get "baz".

Still unsure? Consider this:

import foobar, {foo, default as _default} from './foobar';

What should the value of _default be? "baz", you might claim? Nope. foobar === _default. So why should the value of foo be "bar"? Because someone "wants" it to be? It's not consistent.

You say that what's expected and wanted is for users to be able to import default export object values by that object's keys. What users want is a logical contradiction. Node should not satisfy such requests.

Users want import {a, b, c} to mean "pull those properties off of the 'main' object I am importing." But import {} is not an object destructuring syntax, like var {a, b, c} = obj is. It's more like a "module exports map" destructuring syntax.

If users want to pull properties off of a "main" object, then they can just do that:

import foobar from './foobar';
var {foo, default: _default} = foobar;
foo // => 'bar'
_default // => 'baz'

No special enhancements based-off logical contradictions are required.

If a module author wants to publish code with exports other than default, then he should not distribute CommonJS code, he should distribute ES module code. No detection of magic properties like __esModule inside CommonJS code to make CommonJS code masquerade as ES module code are required.

devsnek · 2017-11-27T15:19:59Z

@jacksonrayhamilton hello and thank you for your input. I appreciate you taking the time to voice your opinion, however there are a few points i would like to bring up in regard to your comment.
First, you spend a large portion discussing how es modules work, which i think everyone here already understands. That being said, Its good to make sure people have knowledge of the mechanics they are discussing, but I think you should be careful not to come across as too condescending.
Secondly, you brush past one of the most important parts of this PR, which is the fact that it is opt in. In the last paragraph you mention that. I would agree in theory that people who want to support es modules should write es modules, but the fact remains that very large codebases that sometimes branch all the way from node 0.10 to the latest versions are being pushed out. Maintaining these is hard enough for huge projects like react or lodash, where they can find people willing to spend the time making sure logical and thoughtful mappings to esm are written, and even in those cases things can go wrong (although i'm sure someone will fix it quick enough, probably with a rocket ship emoji in the pr title), and with smaller projects it might not even be possible. Using standards that are well defined and understood by the community allow us to keep people engaged and making amazing things, as well as leaving them with the opportunity to focus on their code and not writing hundreds of lines of interop for different versions of node.
When all is said and done, this pr is a convenience, and of course it isn't "necessary". But then again, neither is String#repeat 😁.

jacksonrayhamilton · 2017-11-28T07:44:51Z

@devsnek The depth of my explanation is not to imply that I think the authors don't know what they are talking about. (On the contrary, if I'm wrong about modules, then I think this would be one of the best groups of people to convince me.) The depth is to establish, starkly, the false equivalency which is the crux of my argument. In the discussions I've seen on this feature, and from this PR's description, my impression is that "the need" for the feature is "a given." Therefore, I am taking extra care to unearth the problem I see in this being a premise. People often don't expect and react negatively to challenges to an established premise, so I am utilizing even more fundamental knowledge (the mechanics of modules) to make my claim.

I'm curious what you think about the inconsistency I've brought up: Because module.exports is itself imported, therefore module.exports properties do not translate to ES exports.

As for this being an optional feature, I don't think it's good for module authors to be able to opt into this.

The first case I'll address is the one where ES module code is being compiled to CJS, which seems like the most common reason for __esModule to already exist in the wild:

When an author of ES module code compiles ESM to CJS and the compiled CJS code claims to be an ES module, the resulting API is unpleasant for other CJS code requiring the compiled code. Take the most basic case for requiring a module: require('foobar') won't retrieve the default export, which is the "main" export; instead, one must use require('foobar').default to retrieve the "main" export. This is totally unconventional for APIs.

Allowing module authors to opt into an ES module facade for CJS perpetuates the compilation of CJS-unfriendly APIs.

Modules that look like this will live in an src/ directory:

export default function foobar () {}

And will continue to be compiled into modules that look like this in a lib/ directory:

'use strict';
exports.__esModule = true;
function foobar () {}
exports.default = foobar;

And lib/ will continue to serve the package.json "main" module, and require('foobar').default will ensue.

If you require that module authors only deliver ES modules normally, you clear the way for authors to compile CJS-friendly APIs alongside the native ES modules for better backwards compatibility.

Now, a module like this named foobar.mjs can live in some directory:

export var bar = 'qux';
export default function foo () {}

And, compiled alongside it, another file named foobar.js can live:

'use strict';
var bar = 'qux';
function foo () {}
exports = module.exports = foo;
exports.bar = bar;

And import foobar from 'foobar' will yield a function and require('foobar') will also return the same function.

The second case I'll address is the one where an author of originally-CJS code wants to make that code available via an ES module API:

I'm going to assume that most authors of originally-CJS code are not already manually adding exports.__esModule = true to their source code, because __esModule appears at the time when people stop writing CJS and begin writing ESM.

Therefore, these authors have two options:

Manually add __esModule to all their modules (or use a tool to add it automatically), update module.exports = to be exports.default =, and break their CJS users' API with code like require('foobar').default.
Actually update their codebase to ES modules (perhaps using a tool to up-compile), and optionally down-compile at publish time to their original API.

I think the second option is a much better one for authors to follow and consumers to enjoy.

It seems to me this PR would expedite the consumption of today's compiled ES modules, at the expense of the legacy codebases' maintainers for whom we want to make life easier. It looks like a shortcut with drawbacks.

In contrast, my ".mjs and .js sibling" strategy requires a subtle paradigm shift in compilation and distribution methodology, and benefits consumers new and old. As a bonus, Node doesn't need to change.

devsnek · 2017-11-28T13:50:13Z

many libs do module.exports.default = module.exports already for typescript compat. I agree that lib authors moving to esm would be ideal but even with such things as lebab that isn't an easy task

devsnek · 2017-12-12T14:44:16Z

@bmeck any word on the outcome from your topic @ tc39?

bmeck · 2017-12-12T15:03:17Z

@devsnek meeting notes are in queue to be released by end of this week.

bmeck · 2017-12-15T00:32:32Z

@devsnek Notes from TC39 on the topic are out. Reordering is probably not something I would recommend. I'd suggest other routes that don't require reordering be looked at.

devsnek · 2017-12-15T01:03:51Z

@bmeck are there any other ways to achieve this? i wasn't able to think of any

bmeck · 2017-12-15T01:25:56Z

@devsnek not using runtime generated values. alternatives all require some form of preprocessing/parsing. my branch attempting this using a pragma is at https://github.com/bmeck/node/tree/named-export-core , but the cost of using proxies slows down all code that uses the pragma enough to make it undesirable. alternatives generally are going to be in a similar vein to mine and use some form of parsing to get the list of names, then some protocol to setup the export syncing (in my case it was through a Proxy, but more runtime performant measures are probable possible w/ source transforms at the cost of compile time).

devsnek · 2017-12-17T20:30:07Z

i'll just close this as there doesn't seem to be a "safe" way to achieve the wanted behavior 😢. thanks to everyone who joined along for the ride 😄

devsnek · 2017-12-19T21:21:02Z

@bmeck do you know if there's a way to see what names are wanted for a particular import specifier?
e.g.
import x, { y } from 'some cjs';

magicFunction('some cjs', 'parent specifier') // ['default', 'y']

i could parse the source for the import specifiers separately but i don't want to incur the perf penalty

bmeck · 2017-12-19T22:12:37Z

@devsnek you can parse the source and see those. However, even with that the record would be idempotent, so given:

// a
import {x} from './cjs';

// b
import {y} from './cjs';

You would be stuck with one shape of {x} or {y}. It also wouldn't help with import() or import * as cjs ... which pull in all names, which would remain unspecified.

devsnek · 2017-12-19T23:05:50Z

@bmeck i considered that, my current solution is to not put dynamic modules created by the cjs transform in the module map, and use the require cache for idempotence. Obviously the issue them becomes like what if values change with getters or something but then that can be handled by __esModule:
if a module doesn't use the symbol or __esModule, give it only default like the current system, otherwise trust it to not heck with its exports in strange ways

I haven't quite figured out what to do with namespace imports/exports

bmeck · 2017-12-19T23:08:45Z

@devsnek the ECMA262 module map is managed by v8, we can't control it. We only control the loader's module map.

devsnek · 2017-12-19T23:11:30Z

@bmeck yes i meant the loader's module map, wouldn't that make it rerun the transform for each request of the cjs module

bmeck · 2017-12-19T23:14:25Z

@devsnek not for various things like:

import {foo} from './foo'; // this populates a module record with a binding {foo}
import('./foo').then(ns => {
  // ns is the exact same module record as above, unconditionally
})

devsnek · 2017-12-19T23:17:07Z

can we just propose a "ReflectiveModule" with getters and such to the v8 team :(

bmeck · 2017-12-19T23:20:54Z

@devsnek did in the long past: https://github.com/nodejs/node-eps/pull/3/files#diff-0355b3cc08860f0ddbc8338ae885599aR68

addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 2, 2017

add test for exporting reserved keywords from cjs from esm

d3647dd

mcollina reviewed Nov 2, 2017

View reviewed changes

mcollina approved these changes Nov 2, 2017

View reviewed changes

Slight change of plan with loading (use well-defined __esModule)

e8478e2

A very very large portion of the js community (babel/typescript) use `__esModule` to define an es module in CJS.

mcollina requested changes Nov 2, 2017

View reviewed changes

devsnek changed the title ~~loader,docs,test: set named exports based on keys from module.exports~~ loader, docs, test: named exports from commonjs modules Nov 2, 2017

devsnek added 2 commits November 2, 2017 12:49

Symbol('esModule') exported from module builtin

116b470

recommends @@esmodule over __esModule

remove comment i forgot to remove

1a53fd1

mcollina reviewed Nov 2, 2017

View reviewed changes

addaleax reviewed Nov 2, 2017

View reviewed changes

change up symbol usage, use for(;;), add tests

ac2b4a1

mcollina approved these changes Nov 2, 2017

View reviewed changes

guybedford reviewed Nov 2, 2017

View reviewed changes

targos mentioned this pull request Nov 17, 2017

Enable explicit .m.js intent for ESM #16170

Closed

4 tasks

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

devsnek closed this Dec 17, 2017

devsnek deleted the esm-namespace-from-exports branch December 17, 2017 20:30

devsnek restored the esm-namespace-from-exports branch December 19, 2017 21:34

devsnek deleted the esm-namespace-from-exports branch December 19, 2017 21:48

devsnek mentioned this pull request Feb 6, 2018

Online Module Summit 2018-04-03 nodejs/modules#9

Closed

devsnek mentioned this pull request Mar 10, 2018

esm: Implement esm mode flag #18392

Closed

4 tasks

devsnek mentioned this pull request May 14, 2018

Feature: Named exports when importing CJS nodejs/modules#81

Closed

devsnek mentioned this pull request Feb 11, 2019

CJS named exports via two-phase execution nodejs/ecmascript-modules#31

Closed

loader, docs, test: named exports from commonjs modules #16675

loader, docs, test: named exports from commonjs modules #16675

Conversation

devsnek commented Nov 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax commented Nov 2, 2017

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Nov 2, 2017

guybedford commented Nov 2, 2017 • edited Loading

devsnek commented Nov 2, 2017 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Nov 2, 2017

mcollina left a comment

Choose a reason for hiding this comment

devsnek commented Nov 2, 2017 • edited Loading

addaleax commented Nov 2, 2017

devsnek commented Nov 2, 2017

addaleax commented Nov 2, 2017

addaleax commented Nov 2, 2017

devsnek commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Nov 23, 2017

guybedford commented Nov 24, 2017

jacksonrayhamilton commented Nov 27, 2017

devsnek commented Nov 27, 2017

jacksonrayhamilton commented Nov 28, 2017

devsnek commented Nov 28, 2017 • edited Loading

devsnek commented Dec 12, 2017

bmeck commented Dec 12, 2017

bmeck commented Dec 15, 2017

devsnek commented Dec 15, 2017

bmeck commented Dec 15, 2017

devsnek commented Dec 17, 2017

devsnek commented Dec 19, 2017 • edited Loading

bmeck commented Dec 19, 2017

devsnek commented Dec 19, 2017 • edited Loading

bmeck commented Dec 19, 2017

devsnek commented Dec 19, 2017

bmeck commented Dec 19, 2017

devsnek commented Dec 19, 2017

bmeck commented Dec 19, 2017

devsnek commented Nov 2, 2017 •

edited

Loading

guybedford commented Nov 2, 2017 •

edited

Loading

devsnek commented Nov 2, 2017 •

edited

Loading

devsnek commented Nov 2, 2017 •

edited

Loading

devsnek commented Nov 28, 2017 •

edited

Loading

devsnek commented Dec 19, 2017 •

edited

Loading

devsnek commented Dec 19, 2017 •

edited

Loading