Check external links in the main loop #143
Conversation
lib/index.js
Outdated
| } | ||
| } else { | ||
| follow = true; | ||
| } | ||
|
|
||
| if (follow) { | ||
| if (follow || metadataOnly) { | ||
| if (assetTypesWithoutRelations.includes(relation.to.type)) { | ||
| // If we are handling local file-urls, follow but mark as end-of-line in processing | ||
| if (!recursive && relation.from.protocol === 'file:' && relation.to.protocol === 'file:') { |
There was a problem hiding this comment.
This branch can probably die if assetgraph handles the loading
There was a problem hiding this comment.
Fixed in fbaa61b
test/index.js
Outdated
| operator: 'content-type-missing', | ||
| name: 'content-type-missing https://example.com/hey.png', | ||
| expected: 'image/png', | ||
| operator: 'error', |
There was a problem hiding this comment.
I don't like the generic error operator. Can't be properly filtered on. We should probably handle that error explicitly and trigger the right operator
|
I don't think the separate phase and message about switching to outgoing urls is super important. If there's no good way to get it back I'm fine with dropping it |
papandreou
force-pushed
the
feature/loadMetadataOnly
branch
3 times, most recently
from
742d302 to
a94ed21
Compare
…({ metadataOnly: true })
papandreou
force-pushed
the
feature/loadMetadataOnly
branch
from
a94ed21 to
f489f19
Compare
Instead of
push(
null,
{
ok: true,
operator: 'external-redirect',
name: 'external-redirect https://elsewhere.com/',
at: 'https://example.com/ (1:39) <script src="https://elsewhere.com/">...</script>',
expected: '302 https://elsewhere.com/ --> 200 http://elsewhere.com/redirectTarget'
}
); at reportTest (lib/index.js:104:9)
push(
null,
{
ok: true,
operator: 'external-redirect',
name: 'external-redirect http://elsewhere.com/redirectTarget',
at: 'https://elsewhere.com/',
expected: '302 http://elsewhere.com/redirectTarget --> 200 https://elsewhere.com/redirectTarget'
}
); at reportTest (lib/index.js:104:9)
we now get
push(
null,
{
ok: false,
operator: 'external-redirect',
name: 'external-redirect https://elsewhere.com/',
at: 'https://example.com/ (1:39) <script src="https://elsewhere.com/">...</script>',
expected: '302 https://elsewhere.com/ --> 200 https://elsewhere.com/redirectTarget',
actual: '302 https://elsewhere.com/ --> 302 http://elsewhere.com/redirectTarget --> 200 https://elsewhere.com/redirectTarget'
}
); at reportTest (lib/index.js:114:7)
which is correct
…at a new relation came about
…stopProcessing)
This is now handled in assetgraph by having asset.load({metadataOnly: true})
issue an fs.stat call.
#143 (comment)
|
@Munter, I got it working now and addressed the feedback :) |
…ts that have only had their metadata loaded
|
@Munter, could you take another look at this? |
…hough the asset would normally not be populated. Same as #147
|
I tried adding the current functionality from This should put the branch on par with |
|
Great! The solution for that problem looks fine. |
|
@Munter, I'd like to get hyperlink upgraded to assetgraph 5+ and fix that bug we ran into while checking unexpected.js.org: https://gitter.im/unexpectedjs/unexpected?at=5c2e6fba5a0a8058be23948d That weird thing where hyperlink complains about the response body of the HTTP redirect for an image: First it would be nice to get this PR landed as it makes some quite fundamental changes. It's now conflicting with a new feature that landed on master. If you are otherwise OK with this PR I can fix that up? |
TODO:
Maybe reintroduce the "Crawling X outgoing urls" message that I've commented out in the tests. The problem is that we don't have an up front count because it doesn't happen in a separate phase now. Maybe we could just switch it to the past tense ("Crawled X outgoing urls") and output it at the end?asset.keepUnpopulateddoes not block the discovery of outgoingHttpRedirectandFileRedirectrelations