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

hasparus · 2023-03-04T13:51:50Z

PR Checklist

Addresses an existing open issue: fixes Website: Sync Supported Rules filter state in the URL #6171
That issue was marked as accepting prs
Steps in Contributing 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,
and extension-rules.

Each value is list of categories concatenated with dashes -.

If FilterMode is "include", we include the category as is

supported-rules=recommended-fixable

If FilterMode is "exclude", we prefix it with x. (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 a useEffect to update search params after the state change, and useIsomorphicLayoutEffect, 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 the changeFilter function exported from useRuleFilters hook.

We need useIsomorphicLayoutEffect 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 in 7c2e81a I removed setState and synchronization and move the entire state to useLocation, 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

nx-cloud · 2023-03-04T13:51:52Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0eb7299. 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 💌 from NxCloud.

typescript-eslint · 2023-03-04T13:51:53Z

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.

netlify · 2023-03-04T13:51:55Z

✅ Deploy Preview for typescript-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 your Netlify site settings.

hasparus · 2023-03-04T19:23:31Z

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

JoshuaKGoldberg · 2023-03-08T22:46:53Z

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

Yeah 😞. #6508

JoshuaKGoldberg · 2023-03-08T22:47:40Z

packages/website/tests/rules.spec.ts

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


😍 thanks for adding this in!

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

JoshuaKGoldberg

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

Just one suggestion on refactoring RulesTable<>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.

JoshuaKGoldberg · 2023-03-11T23:29:50Z

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

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


[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 the RulesTable component a mandatory id prop that's then passed directly to useRulesFilters?

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

<RulesTable id="extension-rules">

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

How about a prop ruleset: "supported-rules" | "extension-rules" that would subsume the boolean flag and be usable as a search param key?

Yeah I like that a lot!

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

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

nx-cloud · 2023-03-14T06:34:14Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d37c066. 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 💌 from NxCloud.

hasparus · 2023-03-17T09:22:11Z

@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 a contact button at https://app.netlify.com/sites/typescript-eslint/deploys/64131d041e22810009b38f2c.
Who should I bother when the Netlify GitHub integration acts out?

Josh-Cena

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

Josh-Cena · 2023-03-17T20:21:28Z

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

+  const [state, setState] = useState(neutralFiltersState);
+
+  useIsomorphicLayoutEffect(() => {
+    const search = new URLSearchParams(window.location.search);


You can replace window.location with useLocation() so you get SSR safety for free

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

So using location from useLocation here wouldn't be the same logic, right?
And I'm in a useLayoutEffect which does not run on the server, so accessing window.location works.

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

Notice that I had rewritten this logic to react-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 with setState and useLayoutEffect, 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 use useHistorySelector presented in the post?

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 use useIsomorphicLayoutEffect and read from a client-only API inside. useLocation is available both server-side and client-side, so it's more robust.

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 use useState, 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 from 7c2e81a? and write end-to-end tests to ensure it works?

JoshuaKGoldberg · 2023-03-18T23:05:20Z

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

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

packages/website/src/components/lib/useHistorySelector.ts

JoshuaKGoldberg · 2023-03-22T18:22:36Z

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 😞

This reverts commit 7c2e81a.

armano2

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

JoshuaKGoldberg

🚀 excellent, thanks @hasparus!

hasparus · 2023-03-30T12:43:15Z

Thanks for the review and bearing with me, guys 🙏

Skn0tt · 2023-04-03T11:53:40Z

@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 :)

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

JoshuaKGoldberg reviewed Mar 8, 2023

View reviewed changes

JoshuaKGoldberg previously approved these changes Mar 11, 2023

View reviewed changes

JoshuaKGoldberg added the 1 approval One team member has approved this PR; a second should be enough to merge it label Mar 11, 2023

bradzacher added the package: website Issues 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 response Issues waiting for a reply from the OP or another party label Mar 13, 2023

hasparus dismissed JoshuaKGoldberg’s stale review via 642031f March 14, 2023 06:30

hasparus requested a review from JoshuaKGoldberg March 14, 2023 16:55

Josh-Cena reviewed Mar 17, 2023

View reviewed changes

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 21, 2023

hasparus requested review from Josh-Cena and removed request for JoshuaKGoldberg March 22, 2023 14:35

hasparus force-pushed the main branch from 954d808 to 5546b6f Compare March 22, 2023 14:36

hasparus commented Mar 22, 2023

View reviewed changes

packages/website/src/components/RulesTable/index.tsx Show resolved Hide resolved

armano2 reviewed Mar 22, 2023

View reviewed changes

packages/website/src/components/lib/useHistorySelector.ts Outdated Show resolved Hide resolved

hasparus force-pushed the main branch from 5b3ba22 to 3a0723b Compare March 22, 2023 19:53

hasparus added 3 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 commit 7c2e81a.

hasparus added 4 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 the main branch from 3a0723b to 3ca24e1 Compare March 22, 2023 19:55

armano2 previously approved these changes Mar 23, 2023

View reviewed changes

Fix lint errors

d37c066

hasparus dismissed armano2’s stale review via d37c066 March 23, 2023 22:51

JoshuaKGoldberg approved these changes Mar 24, 2023

View reviewed changes

JoshuaKGoldberg merged commit d8e563b into typescript-eslint:main Mar 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hasparus commented Mar 4, 2023 •

edited

nx-cloud bot commented Mar 4, 2023 •

edited

typescript-eslint bot commented Mar 4, 2023

netlify bot commented Mar 4, 2023 •

edited

hasparus commented Mar 4, 2023

JoshuaKGoldberg commented Mar 8, 2023

JoshuaKGoldberg Mar 8, 2023

JoshuaKGoldberg left a comment

JoshuaKGoldberg Mar 11, 2023

hasparus Mar 14, 2023

JoshuaKGoldberg Mar 14, 2023

hasparus Mar 14, 2023 •

edited

JoshuaKGoldberg Mar 22, 2023

nx-cloud bot commented Mar 14, 2023 •

edited

hasparus commented Mar 17, 2023 •

edited

Josh-Cena left a comment

Josh-Cena Mar 17, 2023

hasparus Mar 18, 2023

Josh-Cena Mar 18, 2023 •

edited

hasparus Mar 19, 2023 •

edited

JoshuaKGoldberg commented Mar 18, 2023

JoshuaKGoldberg commented Mar 22, 2023

armano2 left a comment

JoshuaKGoldberg left a comment

hasparus commented Mar 30, 2023

Skn0tt commented Apr 3, 2023

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

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

Conversation

hasparus commented Mar 4, 2023 • edited

PR Checklist

Overview

Solution

Non-solutions

Alternative solution

Example

nx-cloud bot commented Mar 4, 2023 • edited

☁️ Nx Cloud Report

typescript-eslint bot commented Mar 4, 2023

netlify bot commented Mar 4, 2023 • edited

✅ Deploy Preview for typescript-eslint ready!

hasparus commented Mar 4, 2023

JoshuaKGoldberg commented Mar 8, 2023

JoshuaKGoldberg Mar 8, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 11, 2023

Choose a reason for hiding this comment

hasparus Mar 14, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 14, 2023

Choose a reason for hiding this comment

hasparus Mar 14, 2023 • edited

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 22, 2023

Choose a reason for hiding this comment

nx-cloud bot commented Mar 14, 2023 • edited

☁️ Nx Cloud Report

hasparus commented Mar 17, 2023 • edited

Josh-Cena left a comment

Choose a reason for hiding this comment

Josh-Cena Mar 17, 2023

Choose a reason for hiding this comment

hasparus Mar 18, 2023

Choose a reason for hiding this comment

Josh-Cena Mar 18, 2023 • edited

Choose a reason for hiding this comment

hasparus Mar 19, 2023 • edited

Choose a reason for hiding this comment

JoshuaKGoldberg commented Mar 18, 2023

JoshuaKGoldberg commented Mar 22, 2023

armano2 left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

hasparus commented Mar 30, 2023

Skn0tt commented Apr 3, 2023

hasparus commented Mar 4, 2023 •

edited

nx-cloud bot commented Mar 4, 2023 •

edited

netlify bot commented Mar 4, 2023 •

edited

hasparus Mar 14, 2023 •

edited

nx-cloud bot commented Mar 14, 2023 •

edited

hasparus commented Mar 17, 2023 •

edited

Josh-Cena Mar 18, 2023 •

edited

hasparus Mar 19, 2023 •

edited