core(legacy-javascript): use third-party-web for scoring #10849
Conversation
I think it makes sense living in |
| const artifacts = createArtifacts([ | ||
| { | ||
| code: 'String.prototype.repeat = function() {}', | ||
| url: 'https://www.googletagmanager.com/a.js', |
There was a problem hiding this comment.
would be nice to ensure the main entity logic is getting exercised too
There was a problem hiding this comment.
I see us re-testing this particular logic over and over again ... maybe just one test in the lib that this fn will move to?
and just assume that usage within audits is OK (treat like a dependency, for example we don't test that the URL class works as expected in every test)
There was a problem hiding this comment.
Make the Oh lol it's already required I just had it wrong in my head. Carry on! :)mainEntity a required argument, and you've got a deal :)
making it required+typechecking solves exactly my concern, and agreed it would like testing URL at that point.
|
Sorry for my confusion on the required argument part @connorjclark :) WDYT about?
|
|
let's go with |
| * @param {string} url | ||
| * @param {ThirdPartyEntity | undefined} mainDocumentEntity | ||
| */ | ||
| function isFirstParty(url, mainDocumentEntity) { |
There was a problem hiding this comment.
Maybe we can rename this? It isn't really a first party check, it's more of a "not a known majorish third party" check. As an example, up in legacy-javascript.js, URL.rootDomainsMatch(row.url, mainResource.url) is being replaced with a thirdPartyWeb.isFirstParty(row.url, mainDocumentEntity);, which is a better choice for that check but also potentially a very different set of results, depending on the test page.
(removing and doing a if (!thirdPartyWeb.isThirdParty()) up above also seems very clear in its intention if we don't want to bikeshed on names)
| describe('third party web', () => { | ||
| it('basic', () => { | ||
| expect(thirdPartyWeb.isThirdParty('https://www.example.com', undefined)).toBe(false); | ||
| expect(thirdPartyWeb.isFirstParty('https://www.example.com', undefined)).toBe(true); |
There was a problem hiding this comment.
maybe this is a better example of where the naming gets weird. Just reading this in isolation, I would have no idea why https://www.example.com was determined to be first party :)
part of #10369
Still an informative audit, so no real change here.
@patrickhulce you mentioned having to write a
isThirdPartyfunction for a couple things. where do you want to put this fn?