Skip to content
This repository was archived by the owner on Oct 15, 2020. It is now read-only.

Conversation

@kfarnung
Copy link
Contributor

The code currently assumes that console is already writable, but
that's only if it was previously defined as writable. If it hasn't
already been defined then the default value is false.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@kfarnung kfarnung self-assigned this Apr 21, 2018
@kfarnung kfarnung requested a review from MSLaguana April 21, 2018 01:14
@kfarnung
Copy link
Contributor Author

@kfarnung
Copy link
Contributor Author

Ideally I'd like to upstream the lib change, but it probably makes sense to float the patch for now.

@kfarnung
Copy link
Contributor Author

Upstream PR: nodejs/node#20185

Copy link
Contributor

@sethbrenith sethbrenith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Apr 21, 2018
The console property should not be enumerable according to the W3C
spec.

PR-URL: nodejs#518
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Apr 21, 2018
The code currently assumes that `console` is already writable, but
that's only if it was previously defined as writable. If it hasn't
already been defined then the default value is false.

PR-URL: nodejs#518
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Apr 22, 2018
The console property should not be enumerable according to the W3C
spec.

PR-URL: nodejs#518
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Apr 22, 2018
The code currently assumes that `console` is already writable, but
that's only if it was previously defined as writable. If it hasn't
already been defined then the default value is false.

PR-URL: nodejs#518
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
@jackhorton
Copy link
Contributor

This should be closed now, right?

The code currently assumes that `console` is already writable, but
that's only if it was previously defined as writable. If it hasn't
already been defined then the default value is false.

PR-URL: nodejs#518
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
@kfarnung
Copy link
Contributor Author

I had to drop the inspector change, there's something very odd going on that's causing a test break. Just this change should be good enough, so I'll split out the other change and investigate.

@kfarnung
Copy link
Contributor Author

@jackhorton I'm still planning to land this here since the node change will take a couple days to land.

@kfarnung
Copy link
Contributor Author

@kfarnung kfarnung merged commit 0274a79 into nodejs:master Apr 22, 2018
kfarnung added a commit that referenced this pull request Apr 22, 2018
The code currently assumes that `console` is already writable, but
that's only if it was previously defined as writable. If it hasn't
already been defined then the default value is false.

PR-URL: #518
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
@kfarnung kfarnung deleted the console branch April 22, 2018 06:54
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Jun 29, 2018
The console property should not be enumerable according to the W3C
spec.

PR-URL: nodejs#518
Reviewed-By: Seth Brenith <sethb@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants