Skip to content

Conversation

typescript-bot
Copy link
Collaborator

Please review the diff and merge if no changes are unexpected.
You can view the build log here.

cc @weswigham @sandersn @RyanCavanaugh

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Still need to look at a few changes in chrome-devtools-frontend, but I think they're all the result of Object.defineProperty. @weswigham I noticed one limitation of the new code and filed a bug on it.

@@ -582,9 +582,11 @@ node_modules/async/priorityQueue.js(9,14): error TS2695: Left side of comma oper
node_modules/async/priorityQueue.js(18,15): error TS2695: Left side of comma operator is unused and has no side effects.
node_modules/async/priorityQueue.js(23,21): error TS2695: Left side of comma operator is unused and has no side effects.
node_modules/async/priorityQueue.js(47,10): error TS2695: Left side of comma operator is unused and has no side effects.
node_modules/async/priorityQueue.js(86,12): error TS2304: Cannot find name 'AsyncFunction'.
Copy link
Member

Choose a reason for hiding this comment

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

EOF comment checking now finds an error in a bad type reference.

@@ -120,13 +120,11 @@ node_modules/npm/lib/config/bin-links.js(30,16): error TS2339: Property 'config'
node_modules/npm/lib/config/core.js(17,1): error TS2323: Cannot redeclare exported variable 'loaded'.
node_modules/npm/lib/config/core.js(18,1): error TS2323: Cannot redeclare exported variable 'rootConf'.
node_modules/npm/lib/config/core.js(19,1): error TS2323: Cannot redeclare exported variable 'usingBuiltin'.
node_modules/npm/lib/config/core.js(23,21): error TS2339: Property 'defaults' does not exist on type 'typeof import("/npm/node_modules/npm/lib/config/defaults")'.
Copy link
Member

Choose a reason for hiding this comment

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

very likely the new understanding of Object.defineProperty

@@ -6067,6 +6083,13 @@ node_modules/chrome-devtools-frontend/front_end/extensions/ExtensionAPI.js(429,1
node_modules/chrome-devtools-frontend/front_end/extensions/ExtensionAPI.js(529,12): error TS2339: Property '_fire' does not exist on type 'EventSinkImpl'.
node_modules/chrome-devtools-frontend/front_end/extensions/ExtensionAPI.js(544,12): error TS2339: Property '_fire' does not exist on type 'EventSinkImpl'.
node_modules/chrome-devtools-frontend/front_end/extensions/ExtensionAPI.js(551,12): error TS2339: Property '_fire' does not exist on type 'EventSinkImpl'.
node_modules/chrome-devtools-frontend/front_end/extensions/ExtensionAPI.js(768,19): error TS2339: Property 'inspectedWindow' does not exist on type '{}'.
Copy link
Member

Choose a reason for hiding this comment

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

better types, probably defineProperty?

Copy link
Member

Choose a reason for hiding this comment

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

yep, we don't treat defineProperty as an expando assignment, so

var chrome = {}
chrome.devtools = {}
chrome.devtools.inspectedWindow = {}

works, but

var chrome = {}
Object.defineProperty(chrome, 'devtools', { value: {}, enumerable: true })
chrome.devtools.inspectedWindow = {}

does not. Unfortunately, I haven't got a repro that matches the error in chrome; the previous example gives me chrome.devtools: any in a separate buffer, not chrome.devtools: {} as I see on the new error line here.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #28049 to track this.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I'm going to accept these changes so they don't pile up over time, and because both bugs are tracked now.

@sandersn sandersn merged commit 778f438 into microsoft:master Oct 22, 2018
@sandersn
Copy link
Member

Filed #28054 to track the inability to rename defineProperty properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants