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

Various fixes to `forked-to` #2216

Merged
merged 4 commits into from Jul 10, 2019

Conversation

Projects
None yet
2 participants
@bfred-it
Copy link
Collaborator

commented Jul 8, 2019

Follows #2153

Changes:

  • Actually validate fork via HTTP request
  • Don't validate current page
  • Delete cache entry completely when fork is gone
  • Avoid merging with existing cache when unnecessary (i.e. when opening the fork dialog)
  • Get/validate/update cache in one step before showing the links
  • Reduce number of functions

Currently untested

cc @jerone

bfred-it added some commits Jul 8, 2019

@jerone

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

I was trying something like the following to show the forks sooner:

async function load(callback: () => void): Promise<void> {
	await elementReady('.pagehead h1.public');
	callback();
	await features.onAjaxedPages(callback);
}

But this code will ofc show double forks on some pages.

@jerone

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

I tested this and aldo it works, it is noticeable slower. Maybe the validateFork check should be executed after adding the forks HTML, on the cache, so next page load shows the validated forks.

@bfred-it

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

That works. Also I’ll have to avoid revalidation if it was done “recently”

I think a better strategy is to just fetch the fork box if there’s a fork in the cache:

  • it will validate all forks and replenish the cache in just one HTTP request
@jerone

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

I think a better strategy is to just fetch the fork box [..]

The fork fragment? If yes, see https://github.com/jerone/refined-github/blob/9df50080dfddee5f7a2a6a1dc4465166339fedfe/source/features/forked-to.tsx#L7-L12 how to load the fragment.

@bfred-it bfred-it marked this pull request as ready for review Jul 10, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

Now forks will be validated after being shown.

If you want to speed it up further with multiple forks, you can fetch the fork fragment, but for now let's see what happens in #2214

@bfred-it bfred-it merged commit 423304c into master Jul 10, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bfred-it bfred-it deleted the fix-forked-to branch Jul 10, 2019

notlmn added a commit to notlmn/refined-github that referenced this pull request Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.