typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star15.8k

chore(website): preserve RulesTable filters state in searchParams#6568

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

JoshuaKGoldberg merged 8 commits intotypescript-eslint:mainfromhasparus:main

Mar 24, 2023

Merged

chore(website): preserve RulesTable filters state in searchParams#6568

JoshuaKGoldberg merged 8 commits intotypescript-eslint:mainfromhasparus:main

Mar 24, 2023

Conversation

Copy link

Contributor

hasparus commentedMar 4, 2023•
edited
Loading

PR Checklist

Addresses an existing open issue:fixesWebsite: Sync Supported Rules filter state in the URL #6171
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

Howdy! 👋

I added syncing rules table filters state to URL search params, and I wrote two test cases for it.

Solution

There are two tables, so I serialised the state to two search params, one for each table

supported-rules,
andextension-rules.

Each value is list of categories concatenated with dashes-.

IfFilterMode is"include", we include the category as is

supported-rules=recommended-fixable

IfFilterMode is"exclude", we prefix it withx. (We can't use! in the URL.)

supported-rules=xrecommended-strict-xfixable

I grouped the state of filters into one objects so it's easier to stringify.

Non-solutions

I initially just added auseEffect to update search params after the state change, anduseIsomorphicLayoutEffect, but the problem was that the initial state would be written to the URL before it was read. I ended up moving writing to the URL to thechangeFilter function exported fromuseRuleFilters hook.

We needuseIsomorphicLayoutEffect to read from the URL instead of just using the state initialiser, because initialising different state in SSG and post-hydration would give us a hydration mismatch.

Alternative solution

I noticed Docusaurus uses React Router, so in7c2e81a I removedsetState and synchronization and move the entire state touseLocation, what in theory sounded better, because we'd just keep it in one place. Unfortunately, this got me a bug where the state wasn't re-read post hydration (useLocation doesn't rerender again?) and I needed a layout effect again, so I ended up just reverting this change.

Example

Screen.Recording.2023-03-04.at.18.28.04.mov

Copy link

nx-cloudbot commentedMar 4, 2023•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit0eb7299. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 46 targets

Sent with 💌 fromNxCloud.

Copy link

Contributor

typescript-eslintbot commentedMar 4, 2023

Thanks for the PR,@hasparus!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlifybot commentedMar 4, 2023•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`d37c066`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/641cd7e18980310008873bdf
😎 Deploy Preview	https://deploy-preview-6568--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site settings.

hasparus marked this pull request as draft

March 4, 2023 13:51

hasparus marked this pull request as ready for review

March 4, 2023 19:21

Copy link

ContributorAuthor

hasparus commentedMar 4, 2023

It seems that the tests are failing, because they wait for the deploy preview and they timeout before it deploys.

Copy link

Member

JoshuaKGoldberg commentedMar 8, 2023

It seems that the tests are failing, because they wait for the deploy preview and they timeout before it deploys.

Yeah 😞.#6508

JoshuaKGoldberg reviewed

Mar 8, 2023

View reviewed changes

packages/website/tests/rules.spec.ts


		test('Accessibility', async ({ page }) => {
		await new AxeBuilder({ page }).analyze();
		});

Copy link

Member

JoshuaKGoldbergMar 8, 2023

Choose a reason for hiding this comment

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

😍 thanks for adding this in!

(I'll continue reviewing soon, just wanted to express appreciation sooner)

JoshuaKGoldberg previously approved these changes

Mar 11, 2023

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Yup, this looks great. Thanks for sending it in@hasparus!

Just one suggestion on refactoringRulesTable<>useRulesFilters a bit. What do you think?

I'm down to merge as-is if you don't like the suggestion, or make it myself if you don't have time.

packages/website/src/components/RulesTable/index.tsx Outdated

		@@ -122,78 +123,57 @@ export default function RulesTable({
		}: {
		extensionRules?: boolean;
		}): JSX.Element {
		const [filters, changeFilter] = useRulesFilters(
		extensionRules ? 'extension-rules' : 'supported-rules',
		);

Copy link

Member

JoshuaKGoldbergMar 11, 2023

Choose a reason for hiding this comment

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

[Cleanup] I'm not a big fan of hardcoding IDs on boolean logic. It tends to break over time, and makes it harder to understand where the IDs come from. How about giving theRulesTable component a mandatoryid prop that's then passed directly touseRulesFilters?

Copy link

ContributorAuthor

hasparusMar 14, 2023

Choose a reason for hiding this comment

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

I agree about hardcoding is no boolean logic, but I feel that

<RulesTable>

might be a bit confusing given that the heading above receivesid attribute from the markdown pipeline.

How about a propruleset: "supported-rules" | "extension-rules" that would subsume the boolean flagand be usable as a search param key?

Copy link

Member

JoshuaKGoldbergMar 14, 2023

Choose a reason for hiding this comment

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

Yeah I like that a lot!

Copy link

ContributorAuthor

hasparusMar 14, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I added it in642031f, but it seems there are indirect codecov changes that are failing the CI. Do you think it's connected to my change?

Copy link

Member

JoshuaKGoldbergMar 22, 2023

Choose a reason for hiding this comment

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

We can ignore codecov for these website PRs. We don't have great unit test coverage on it anyway. 🤷

JoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

Mar 11, 2023

bradzacher added the package: websiteIssues related to the @typescript-eslint website label

Mar 13, 2023

bradzacher changed the title~~feat(website): preserve RulesTable filters state in searchParams~~chore(website): preserve RulesTable filters state in searchParams

Mar 13, 2023

bradzacher added the awaiting responseIssues waiting for a reply from the OP or another party label

Mar 13, 2023

hasparus dismissedJoshuaKGoldberg’sstale review via642031f

March 14, 2023 06:30

Copy link

nx-cloudbot commentedMar 14, 2023•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitd37c066. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 25 targets

Sent with 💌 fromNxCloud.

hasparus requested a review fromJoshuaKGoldberg

March 14, 2023 16:55

Copy link

ContributorAuthor

hasparus commentedMar 17, 2023•
edited
Loading

@JoshuaKGoldberg The deploy has finished in 3 minutes, but the status check's been hanging for 19 hours. Is there a way we can stop it or restart it? This seems like a bug in the Netlify integration.

@Skn0tt sorry for pinging you here, but I couldn't find acontact button athttps://app.netlify.com/sites/typescript-eslint/deploys/64131d041e22810009b38f2c.
Who should I bother when the Netlify GitHub integration acts out?

Josh-Cena reviewed

Mar 17, 2023

View reviewed changes

Copy link

Member

Josh-Cena left a comment

Choose a reason for hiding this comment

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

The implementation looks mostly good to me, but as some extra reading, you may want to tryuseSyncExternalStore (interesting read) ^^

packages/website/src/components/RulesTable/index.tsx Outdated

		const [state, setState] = useState(neutralFiltersState);

		useIsomorphicLayoutEffect(() => {
		const search = new URLSearchParams(window.location.search);

Copy link

Member

Josh-CenaMar 17, 2023

Choose a reason for hiding this comment

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

You can replacewindow.location withuseLocation() so you get SSR safety for free

Copy link

ContributorAuthor

hasparusMar 18, 2023

Choose a reason for hiding this comment

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

I expecteduseLocation() to trigger re-renders whenever the location changes, as the docs would suggest:https://reactrouter.com/en/main/hooks/use-location

So usinglocation fromuseLocation here wouldn't be the same logic, right?
And I'm in auseLayoutEffect which does not run on the server, so accessingwindow.location works.

Could you tell me more what do you mean by "SSR safety for free"?

Notice that I had rewritten this logic toreact-router specific hooks, what I described in"Alternative solution" in the original post. It turned out to be buggy and harder to reason about than the less elegant, naive solution withsetState anduseLayoutEffect, so I reverted that change —786557f.

The implementation looks mostly good to me, but as some extra reading, you may want to try useSyncExternalStore (interesting read) ^^

Nice article! Do you think I should rewrite this PR to useuseHistorySelector presented in the post?

Copy link

Member

Josh-CenaMar 18, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I'm okay either way! I don't have strong opinions on this, and TBH my React is getting a bit rusty these days. Just thought you may be interested—since effects don't work well with SSR.

Could you tell me more what do you mean by "SSR safety for free"?

I mean you don't have to useuseIsomorphicLayoutEffect and read from a client-only API inside.useLocation is available both server-side and client-side, so it's more robust.

Copy link

ContributorAuthor

hasparusMar 19, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I mean you don't have to use useIsomorphicLayoutEffect and read from a client-only API inside. useLocation is available both server-side and client-side, so it's more robust.

Well, yes, that's true, but then we shouldn't useuseState, because a change to the location will already re-render the component. Would you agree?

Would you prefer if I went back to the state from7c2e81a? and write end-to-end tests to ensure it works?

Copy link

Member

JoshuaKGoldberg commentedMar 18, 2023

(I'll defer to@Josh-Cena - who is more knowledgeable than me on Docusaurus best practices anyway!)

bradzacher removed the awaiting responseIssues waiting for a reply from the OP or another party label

Mar 21, 2023

hasparus requested review fromJosh-Cena and removed request forJoshuaKGoldberg

March 22, 2023 14:35

hasparus force-pushed themain branch from954d808 to5546b6fCompare

March 22, 2023 14:36

hasparus commented

Mar 22, 2023

View reviewed changes

packages/website/src/components/RulesTable/index.tsxShow resolvedHide resolved

armano2 reviewed

Mar 22, 2023

View reviewed changes

packages/website/src/components/lib/useHistorySelector.ts OutdatedShow resolvedHide resolved

Copy link

Member

JoshuaKGoldberg commentedMar 22, 2023

Who should I bother when the Netlify GitHub integration acts out?

Oh sorry I missed this - it's been flaky for a while. We can ignore it.#6508 😞

hasparus force-pushed themain branch from5b3ba22 to3a0723bCompare

March 22, 2023 19:53

hasparus added3 commits

March 22, 2023 20:54

feat(website): preserve RulesTable filters state in searchParams

0f06ead

Move rules filters state to useLocation

015218f

Revert "Move rules filters state to useLocation"

92a9c55

This reverts commit7c2e81a.

hasparus added4 commits

March 22, 2023 20:54

Test rules filters in URL

d3e8ae9

Use .ruleset prop in RulesTable

aa32a3c

Use useHistorySelector instead of useIsomorphicLayoutEffect

5e574a8

Move useHistorySelector to src/hooks

3ca24e1

hasparus force-pushed themain branch from3a0723b to3ca24e1Compare

March 22, 2023 19:55

armano2 previously approved these changes

Mar 23, 2023

View reviewed changes

Copy link

Collaborator

armano2 left a comment

Choose a reason for hiding this comment

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

this looks great, if you fix those linting errors i think we are ready to merge

Fix lint errors

d37c066

hasparus dismissedarmano2’sstale review viad37c066

March 23, 2023 22:51

JoshuaKGoldberg approved these changes

Mar 24, 2023

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🚀 excellent, thanks@hasparus!

JoshuaKGoldberg merged commitd8e563b intotypescript-eslint:main

Mar 24, 2023

Copy link

ContributorAuthor

hasparus commentedMar 30, 2023

Thanks for the review and bearing with me, guys 🙏

Copy link

Skn0tt commentedApr 3, 2023

@Skn0tt sorry for pinging you here

Sorry for getting back so late, I was on PTO the past weeks. Not sure what happened here, but it looks like things are alright now? Let me know if not, and i'll see how I can help :)

github-actionsbot locked asresolvedand limited conversation to collaborators

Apr 11, 2023

Labels

1 approval

>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge

package: website

Issues related to the @typescript-eslint website

6 participants

Movatterモバイル変換

Uh oh!

chore(website): preserve RulesTable filters state in searchParams#6568

chore(website): preserve RulesTable filters state in searchParams#6568

Uh oh!

Conversation

hasparus commentedMar 4, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

PR Checklist

Overview

Solution

Non-solutions

Alternative solution

Example

Uh oh!

nx-cloudbot commentedMar 4, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

typescript-eslintbot commentedMar 4, 2023

Uh oh!

netlifybot commentedMar 4, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

✅ Deploy Preview fortypescript-eslint ready!

Uh oh!

hasparus commentedMar 4, 2023

Uh oh!

JoshuaKGoldberg commentedMar 8, 2023

Uh oh!

JoshuaKGoldbergMar 8, 2023

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergMar 11, 2023

Choose a reason for hiding this comment

Uh oh!

hasparusMar 14, 2023

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergMar 14, 2023

Choose a reason for hiding this comment

Uh oh!

hasparusMar 14, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergMar 22, 2023

Choose a reason for hiding this comment

Uh oh!

nx-cloudbot commentedMar 14, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

hasparus commentedMar 17, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Josh-Cena left a comment

Choose a reason for hiding this comment

Uh oh!

Josh-CenaMar 17, 2023

Choose a reason for hiding this comment

Uh oh!

hasparusMar 18, 2023

Choose a reason for hiding this comment

Uh oh!

Josh-CenaMar 18, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasparusMar 19, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg commentedMar 18, 2023

Uh oh!

Uh oh!

Uh oh!

JoshuaKGoldberg commentedMar 22, 2023

hasparus commentedMar 4, 2023•
edited
Loading

nx-cloudbot commentedMar 4, 2023•
edited
Loading

netlifybot commentedMar 4, 2023•
edited
Loading

hasparusMar 14, 2023•
edited
Loading

nx-cloudbot commentedMar 14, 2023•
edited
Loading

hasparus commentedMar 17, 2023•
edited
Loading

Josh-CenaMar 18, 2023•
edited
Loading

hasparusMar 19, 2023•
edited
Loading