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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warp speed 馃殌 #6006

Merged
merged 17 commits into from
Sep 25, 2022
Merged

Warp speed 馃殌 #6006

merged 17 commits into from
Sep 25, 2022

Conversation

fregante
Copy link
Member

@fregante fregante commented Sep 22, 2022

Previous details. Already addressed

I don't know how much I'll change in this PR, but I'm opening it to keep track of what needs to be reviewed before merging or after:

  • if attachElement are used, then it should:
    • not use deduplicate
  • if observe is used, then it should:
    • use awaitDomReady: false
    • not use el.classList.add('rgh-some-class') unless it has other uses
    • not use additionalListeners
    • be used on elements that are eventually visible, not on meta tags for example
  • if init has signal, then it should:
    • never use onetime(init)
  • if deduplicate was dropped, then it should:
    • use attachElement, OR
    • use another deduplication mechanism, like a classic :not(.rgh-some-class)

And more generically

  • if a feature uses DOM-based detections, it should not use awaitDomReady: false
  • if the feature lacks include, asLongAs, and exclude, it should probably not use signal

All the rules in ripgrep form, which returns a list of "invalid" files: (WIP)

rg -l 'attachElement' (rg -l 'deduplicate')
rg -l '\tobserve\(' (rg -l 'attachElement')
rg -l '\tobserve' (rg --files-without-match 'awaitDomReady')
rg -l '\tobserve' (rg -l 'classList.add')
rg -l '\tobserve' (rg -l 'additionalListeners')
rg -l '\(signal:' (rg -l 'init: onetime') 

Commands:

  • append | xargs code to open the files in vscode instead of just listing them`
  • Prepend $ to the parens if you're not using Fishell or PowerShell

Features already broken:

  • jump-to-change-requested-comment

Copy link
Member Author

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Me, as I progressively delete code and hacks thanks to the new observer

Copy link
Member Author

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Needs testing. 馃ゲ

Primarily we need to understand whether the selector observer alone can be used as a deduplicator across page loads

@@ -284,8 +261,6 @@ const add = async (url: string, ...loaders: FeatureLoader[]): Promise<void> => {
continue;
}

enforceDefaults(id, include, additionalListeners);
Copy link
Member Author

Choose a reason for hiding this comment

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

Dropping all of these defaults is exposing a lot of unnecessary listeners in place for features that don't need them.

@@ -69,7 +69,6 @@
"push-form": "^1.0.1",
"regex-join": "^1.0.0",
"select-dom": "^7.1.1",
"selector-observer": "^2.1.6",
Copy link
Member Author

Choose a reason for hiding this comment

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

this.

@fregante fregante marked this pull request as ready for review September 22, 2022 13:01
Copy link
Member Author

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Time to merge and release! All features were more or less tested over the past days.

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