[v11.x] backport console related changes #25420
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
From the WHATWG console spec:
> For historical web-compatibility reasons, the namespace object for
> console must have as its [[Prototype]] an empty object, created as
> if by ObjectCreate(%ObjectPrototype%), instead of %ObjectPrototype%.
Since in Node.js, the Console constructor has been exposed through
require('console'), we need to keep the Console constructor but
we cannot actually use `new Console` to construct the global console.
This patch changes the prototype chain of the global console object,
so the console.Console.prototype is not in the global console prototype
chain anymore.
```
const proto = Object.getPrototypeOf(global.console);
// Before this patch
proto.constructor === global.console.Console
// After this patch
proto.constructor === Object
```
But, we still maintain that
```
global.console instanceof global.console.Console
```
through a custom Symbol.hasInstance function of Console that tests
for a special symbol kIsConsole for backwards compatibility.
This fixes a case in the console Web Platform Test that we commented
out.
PR-URL: nodejs#23509
Refs: whatwg/console#3
Refs: https://console.spec.whatwg.org/#console-namespace
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Specifically for v11.x. Refs: nodejs#23509
In 6223236 we made the console.Console function construct an object with methods from Console.prototype bound to the instance, instead of with methods found on the prototype chain to be bound on the instances, thus breaking users overriding these methods when subclassing Console because they are not expecting this function to construct a namespace whose methods are not looked up from the prototype chain. This patch restores the previous behavior since it does not affect the characteristics of the global console anyway. So after this patch, the Console function still constructs normal objects with function properties looked up from the prototype chain but bound to the instances always. PR-URL: nodejs#24047 Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This patch: - Refactors the Console constructor: moves the property binding code into and the writable streams binding code into two methods defined on the Console.prototype with symbols. - Refactors the global console creation: we only need to share the property binding code from the Console constructor. To bind the streams we can lazy load `process.stdio` and `process.stderr` so that we don't create these streams when they are not used. This significantly reduces the number of modules loaded during bootstrap. Also, by calling the refactored-out method directly we can skip the unnecessary typechecks when creating the global console and there is no need to create a temporary Console anymore. - Refactors the error handler creation and the `write` method: use a `kUseStdout` symbol to tell the internals which stream should be loaded from the console instance. Also put the `write` method on the Console prototype so it just loads other properties directly off the console instance which simplifies the call sites. Also leaves a few TODOs for further refactoring of the console bootstrap. PR-URL: nodejs#24534 Reviewed-By: Gus Caplan <me@gus.host>
nodejs-github-bot
added
console
|
@joyeecheung thanks a lot! I believe the second commit should be squashed into the first one while landing. Otherwise there would theoretically be one commit which would contain the breaking change in v11 (which might not be horrible?). |
|
Landed in 453bd18...4052aec 🎉 |
Merged
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this pull request
Jan 16, 2019
Specifically for v11.x. PR-URL: nodejs#25420 Refs: nodejs#23509 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport #23509 #24047 and #24534 to v11.x
The second commit was added specifically for v11.x
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes