Skip to content
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

Fix problem with failing fragment check on non-loaded external html assets #147

Merged
merged 3 commits into from
Apr 10, 2018

Conversation

Munter
Copy link
Owner

@Munter Munter commented Apr 10, 2018

As talked about in gitter, anchors (and similar) to external assets, which contain fragment identifiers, failed the fragment-check test because the external asset was never loaded.

  • Mark the external asset for loading if the relation has a fragment
  • Collect the assets id's before stopping the further processing

@Munter Munter requested a review from papandreou April 10, 2018 19:13
@Munter
Copy link
Owner Author

Munter commented Apr 10, 2018

This seems to be a performance regression. I'm testing out what happens if I immediately unload the asset before returning when hitting https://github.com/Munter/hyperlink/pull/147/files#diff-6d186b954a58d5bb740f73d84fe39073R484

@Munter
Copy link
Owner Author

Munter commented Apr 10, 2018

Ok, seems I had a whole bunch of network timeouts, probably due to local conditions. Also possibly from loading the extra external assets.

I'm finding true positives on the external fragment checks now 👯‍♂️

Copy link
Collaborator

@papandreou papandreou left a comment

Choose a reason for hiding this comment

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

Looks good!

It would help my efforts over at #143 if the test could also assert that outgoing relations from
https://example2.com/foo.html aren't followed.

@Munter
Copy link
Owner Author

Munter commented Apr 10, 2018

I'll extend the test with a few outgoing extras without allowing the network traffic

…was loaded due to incoming fragment identifier relations
@Munter Munter merged commit 070916d into master Apr 10, 2018
@Munter Munter deleted the fix-load-externals-with-incoming-fragments branch April 10, 2018 21:21
Munter added a commit that referenced this pull request Jul 23, 2018
…hough the asset would normally not be populated. Same as #147
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.

None yet

2 participants