polish(theme-classic): guard against potential definition mistakes in Tabs #5674

Josh-Cena · 2021-10-09T10:19:05Z

Breaking changes

Tabs fail-fast (throw) instead of fail-safe when suspicious (likely typo) config is encountered
Use defaultValue={null} if you want to tabs initially selected

Motivation

Followup from here: #5647 (comment)

Have you read the Contributing Guidelines on pull requests?

Yes

netlify · 2021-10-09T10:25:03Z

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 6b76bc9

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6167fc0fef601400075a9148

😎 Browse the preview: https://deploy-preview-5674--docusaurus-2.netlify.app

github-actions · 2021-10-09T10:25:18Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	74
🟢 Accessibility	98
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	95

Lighthouse ran on https://deploy-preview-5674--docusaurus-2.netlify.app/

packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx

slorber

LGTM

Just some missing tests and conflicts to resolve.

What's the behavior if defaultValue=undefined ? different than null?

jest.config.js

packages/docusaurus-theme-classic/src/theme/Tabs/__tests__/index.test.tsx

packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx

Josh-Cena · 2021-10-13T23:13:45Z

What's the behavior if defaultValue={undefined}?

That's just the same as not providing the prop: we search in the children for default or default to the first tab.

slorber

almost LGTM thanks

slorber · 2021-10-14T08:07:56Z

packages/docusaurus-theme-common/src/utils/jsUtils.ts

+ * @param comparator Compares two values and returns `true` if they are equal (duplicated).
+ * @returns Value of the elements `v` that have a preceding element `u` where `comparator(u, v) === true`. Values within the returned array are not guaranteed to be unique.
+ */
+export function getDuplicateValues<T>(


I'd suggest reusing method names from lodash for simplicity: intersection / intersectionBy would be a more powerful abstraction:

https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_intersection

Also adding a little test for it with/without iteratee/comparator

Actually—I've never used Lodash myself and I kind of feel lucky that I never got into Lodash's mindset🌚

I fail to see how intersection is related to getDuplicateValues though, since mine is only working on one array

Actually—I've never used Lodash myself and I kind of feel lucky that I never got into Lodash's mindset🌚

Not sure what you mean here. Lodash is a bit heavy for frontend but it remains a powerful abstraction to achieve many data transformations. JS is improving and adding some Lodash-like methods over time.

I fail to see how intersection is related to getDuplicateValues though, since mine is only working on one array

Oh you are right I'm dumb 🤪 this is a different API and Lodash doesn't have anything for duplicates

Lodash is a bit heavy for frontend but it remains a powerful abstraction to achieve many data transformations.

Yeah, just meant that when I think of algorithms, I directly think in ES array methods, not the Lodash arsenal.

What I mean is that they thought about method names carefully. If something does not exist in JS and exist in Lodash, reusing method name of lodash (or at least getting some inspiration) is not a bad idea.

There's nothing related to duplicates in lodash afaik so not applicable in this case.

Still I'd probably not name it as getDuplicateValues because it's not a method name that looks like any lodash or native JS array method name either. I'd rather name it simply duplicates

Makes sense

slorber · 2021-10-14T08:10:40Z

packages/docusaurus-theme-classic/src/theme/Tabs/__tests__/index.test.tsx

+        </>,
+      );
+    }).toMatchInlineSnapshot(`[Function]`); // This is just a check that the function returns. We don't care about the actual DOM.
+  });


What about adding a test to cover that <CustomTabItem value="1"/> would lead to the same final JSX (non-shallow) than <TabItem value="1"/>

I don't think we dogfood this case on our site yet so it's worth adding it to ensure this behavior remains possible over time

Thanks for adding those tests BTW, we need to start somewhere 😅

I don't really understand what magic a CustomTabItem could do🧐 There's no styling / logic here anyways—is there a use-case?

Users could decorate a tab with a BrowserWindow comp or whatever they want, and add extra logic if they want.

This should work (and used to work)

<CustomTabs> <CustomTabItem value="v1">Tab 1</CustomTabItem> <CustomTabItem value="v2">Tab 2</CustomTabItem> </CustomTabs>

This should also work but would be more verbose:

<Tabs> <TabItem value="v1"> <BrowserWindow>Tab 1</BrowserWindow> </TabItem> <TabItem value="v2"> <BrowserWindow>Tab 2</BrowserWindow> </TabItem> </Tabs>

I'd like to add a new MDX infra so that themes and sites can register MDX shortcodes without any need to import anything:

<tabs> <tab-item value="v1" link="https://tab1link.com"> Tab 1 </tab-item> <tab-item value="v2" link="https://tab2link.com"> Tab 2 </tab-item> </tabs>

tab-item could be replaced by MDX by:

function CustomTabItem({ children, link }) { return ( <TabItem> <BrowserWindow> <div>{children}</div> <footer> <Link to={link}>Tab link</Link> </footer> </BrowserWindow> </TabItem> ); }

That's why we must ensure that this behavior is preserved other time

Okay, I see. react-test-renderer renders the JSX to actual DOM, so if we are to verify the rendering result, we get HTML. Do you just want to check it gets accepted, or do you want the actual DOM to match (which I can't see... why it would not)?

Actually, as I went down the rabbit hole, verifying it works and getting the DOM structure is non-trivial, because it's hooked into complicated things like docusaurus context and some scroll position utils🧐 I'm struggling with making it all work. Maybe if we are to improve the testing infra, we should create a dummy Layout?

Let's move on, I'd like to release soon, we'll figure out how to test this later and provide better test infra for the whole theme. This likely requires some extra refactorings

slorber · 2021-10-14T13:46:29Z

packages/docusaurus-theme-classic/src/theme/Tabs/__tests__/index.test.tsx

+        </>,
+      );
+    }).toMatchInlineSnapshot(`[Function]`); // This is just a check that the function returns. We don't care about the actual DOM.
+  });


Let's move on, I'd like to release soon, we'll figure out how to test this later and provide better test infra for the whole theme. This likely requires some extra refactorings

Josh-Cena requested review from lex111 and slorber as code owners October 9, 2021 10:19

Josh-Cena changed the title ~~polish(theme-classic): error if defaultValue is nonexistent~~ polish(theme-classic): error if defaultValue is nonexistent + check for children type Oct 9, 2021

Josh-Cena changed the title ~~polish(theme-classic): error if defaultValue is nonexistent + check for children type~~ polish(theme-classic): make Tabs error if defaultValue is nonexistent + check for children type Oct 9, 2021

lex111 reviewed Oct 9, 2021

View reviewed changes

packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx Outdated Show resolved Hide resolved

Josh-Cena force-pushed the strict-tabs branch from 8fc12db to c08a71b Compare October 9, 2021 14:38

Josh-Cena changed the title ~~polish(theme-classic): make Tabs error if defaultValue is nonexistent + check for children type~~ polish(theme-classic): guard against potential definition mistakes in Tabs Oct 9, 2021

Josh-Cena force-pushed the strict-tabs branch from 916ad4e to 1fa5775 Compare October 10, 2021 01:13

slorber added pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: new feature This PR adds a new API or behavior. labels Oct 13, 2021

slorber requested changes Oct 13, 2021

View reviewed changes

jest.config.js Outdated Show resolved Hide resolved

packages/docusaurus-theme-classic/src/theme/Tabs/__tests__/index.test.tsx Show resolved Hide resolved

packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx Outdated Show resolved Hide resolved

facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 13, 2021

Josh-Cena added 3 commits October 14, 2021 07:09

polish(theme-classic): error if defaultValue is nonexistent

1504a11

Check for children type

d780788

Simplify checking

c399614

Josh-Cena force-pushed the strict-tabs branch from 1fa5775 to c399614 Compare October 13, 2021 23:11

Josh-Cena added 2 commits October 14, 2021 08:57

Separate jsUtils

71e5bf6

Add a few tests

6f20c5c

slorber requested changes Oct 14, 2021

View reviewed changes

Josh-Cena added 2 commits October 14, 2021 17:35

Rename util function

70d65ff

Fix test

6b76bc9

slorber approved these changes Oct 14, 2021

View reviewed changes

slorber merged commit c8739ec into facebook:main Oct 14, 2021

Josh-Cena deleted the strict-tabs branch October 14, 2021 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

polish(theme-classic): guard against potential definition mistakes in Tabs #5674

polish(theme-classic): guard against potential definition mistakes in Tabs #5674

Josh-Cena commented Oct 9, 2021 •

edited by slorber

Loading

netlify bot commented Oct 9, 2021 •

edited

Loading

github-actions bot commented Oct 9, 2021 •

edited

Loading

slorber left a comment •

edited

Loading

Josh-Cena commented Oct 13, 2021

slorber left a comment

slorber Oct 14, 2021

Josh-Cena Oct 14, 2021

slorber Oct 14, 2021

Josh-Cena Oct 14, 2021

slorber Oct 14, 2021

Josh-Cena Oct 14, 2021

slorber Oct 14, 2021

Josh-Cena Oct 14, 2021

slorber Oct 14, 2021 •

edited

Loading

Josh-Cena Oct 14, 2021

Josh-Cena Oct 14, 2021

slorber Oct 14, 2021

slorber Oct 14, 2021

polish(theme-classic): guard against potential definition mistakes in Tabs #5674

polish(theme-classic): guard against potential definition mistakes in Tabs #5674

Conversation

Josh-Cena commented Oct 9, 2021 • edited by slorber Loading

Breaking changes

Motivation

Have you read the Contributing Guidelines on pull requests?

netlify bot commented Oct 9, 2021 • edited Loading

github-actions bot commented Oct 9, 2021 • edited Loading

slorber left a comment • edited Loading

Choose a reason for hiding this comment

Josh-Cena commented Oct 13, 2021

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josh-Cena commented Oct 9, 2021 •

edited by slorber

Loading

netlify bot commented Oct 9, 2021 •

edited

Loading

github-actions bot commented Oct 9, 2021 •

edited

Loading

slorber left a comment •

edited

Loading

slorber Oct 14, 2021 •

edited

Loading