-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
misc: remove some unwanted TODOs #14353
Conversation
core/test/config/config-test.js
Outdated
@@ -409,9 +409,6 @@ describe('Config', () => { | |||
}), function(err) { | |||
// We're expecting not to find parent class Audit, so only reject on our | |||
// own custom locate audit error, not the usual MODULE_NOT_FOUND. | |||
// TODO(esmodules): Test migration note: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was already fixed actually
core/lib/url-shim.js
Outdated
@@ -270,5 +270,4 @@ URLShim.INVALID_URL_DEBUG_STRING = | |||
'Lighthouse was unable to determine the URL of some script executions. ' + | |||
'It\'s possible a Chrome extension or other eval\'d code is the source.'; | |||
|
|||
// TODO(esmodules): don't use default export | |||
export default URLShim; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be a named export and used as URLShim everywhere (to denote it isn't the global node URL)? atm we just kinda shadow the global value in all the imports. so not 100% sure about ignoring this TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not 100% sure we could just leave it as a normal TODO
but then we will probably never get to it 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#14182 accidentally deleted the comment here about "remove this (from the build shimming) eventually". should be fine to remove now, and then make URLShim be called literally anything else. Could we maybe even remove the extending from URL and rename the things url-utils (and export its methods)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this needs to change to use global URL https://github.com/googleads/publisher-ads-lighthouse-plugin/blob/2687c2335942adb99ddeafcf6f5e3f92a8079f01/lighthouse-plugin-publisher-ads/audits/loads-gpt-from-official-source.js#L21
@brendankenny cc in case you know of more that needs to be done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also just keep export const URL = globalThis.URL;
as its pretty minimal... and there could be dependencies using url
that are out of our control. anyway, this is a separate thing.
core/lib/url-shim.js
Outdated
@@ -270,5 +270,4 @@ URLShim.INVALID_URL_DEBUG_STRING = | |||
'Lighthouse was unable to determine the URL of some script executions. ' + | |||
'It\'s possible a Chrome extension or other eval\'d code is the source.'; | |||
|
|||
// TODO(esmodules): don't use default export | |||
export default URLShim; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not 100% sure we could just leave it as a normal TODO
but then we will probably never get to it 🤷
7ce03a1
to
7b8393a
Compare
These won't be acted on. ref #12689