misc: remove some unwanted TODOs #14353
Conversation
core/test/config/config-test.js
Outdated
| }), 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.
this was already fixed actually
core/lib/url-shim.js
Outdated
| '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.
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.
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.
#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.
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.
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
| '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.
If not 100% sure we could just leave it as a normal TODO but then we will probably never get to it 🤷
connorjclark
force-pushed
the
rm-comments
branch
from
7ce03a1 to
7b8393a
Compare
3 participants
These won't be acted on. ref #12689