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

Run 50 more features before the whole page has loaded #6357

Merged
merged 7 commits into from
Feb 20, 2023
Merged

Conversation

fregante
Copy link
Member

By default, features no longer wait for DOM-ready. This makes slower features more visible/painful in code

@fregante fregante changed the title Run 70 more features before the whole page has loaded Run 50 more features before the whole page has loaded Feb 19, 2023
@@ -21,6 +20,10 @@ function init(): void {
);
}

function init(signal: AbortSignal): void {
observe('a#raw-url', add, {signal});
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The feature is actually broken at the moment:

@@ -6,6 +6,8 @@ import delegate, {DelegateEvent} from 'delegate-it';
import features from '../feature-manager';

async function handleErroredImage({delegateTarget}: DelegateEvent<ErrorEvent, HTMLImageElement>): Promise<void> {
console.log('Refined GitHub: image failed loading, will retry', delegateTarget.src);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm adding some logging to see if this feature is ever needed anymore

@@ -125,5 +125,6 @@ void features.add(import.meta.url, {
exclude: [
pageDetect.isClosedPR,
],
awaitDomReady: false,
Copy link
Member Author

Choose a reason for hiding this comment

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

There are a couple of features that have DOM-based filters AND don't await dom-ready. I only kept those that aren't fundamental, like this exclusion. I prefer a faster feature to a console without errors. This can be fixed in a better way (e.g. move this check inside the event listener)

Same goes for features that use isBlank as an exclusion: as mentioned in #5222, the manager should instead catch the error and silence it on blank pages.

@@ -6,7 +6,7 @@ import {wrap} from '../helpers/dom-utils';
import features from '../feature-manager';

function init(): void {
const element = select('.sha.user-select-contain');
const element = select('.sha.user-select-contain:not(a *)');
Copy link
Member Author

Choose a reason for hiding this comment

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

Automatic "don't rewrap links" deduplication logic

@fregante fregante marked this pull request as ready for review February 19, 2023 11:56
@fregante
Copy link
Member Author

Will merge this tomorrow and create a new release probably

@fregante fregante merged commit d4b9cd0 into main Feb 20, 2023
@fregante fregante deleted the faster-features branch February 20, 2023 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

1 participant