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.

Sign up for GitHub

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

fix(v2): navbar dropdown crash when item.to is undefined #3662

Merged
merged 4 commits into from
Nov 2, 2020
Merged

fix(v2): navbar dropdown crash when item.to is undefined #3662

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4972f12
Fix for undefined path with dropdown menu in Navbar
artemkovalyov Oct 30, 2020
571583b
Filter out 'href' links and check for undefined in utils
artemkovalyov Nov 1, 2020
d88d4da
Minot syntax fixes
artemkovalyov Nov 1, 2020
1724fee
allow isSamePath to accept undefined values
slorber Nov 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/docusaurus-theme-classic/src/__tests__/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,7 @@ describe('isSamePath', () => {
test('should be false for compared path with double trailing slash', () => {
expect(isSamePath('/docs', '/docs//')).toBeFalsy();
});
test('should not fail if path is "undefined" when it is absent in case of dropdown in Navbar ', () => {
expect(isSamePath(undefined, undefined)).toBeTruthy();
});
});
2 changes: 1 addition & 1 deletion packages/docusaurus-theme-classic/src/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@
// Compare the 2 paths, ignoring trailing /
export const isSamePath = (path1, path2) => {
const normalize = (str) => (str.endsWith('/') ? str : `${str}/`);
return normalize(path1) === normalize(path2);
return path1 && path2 ? normalize(path1) === normalize(path2) : true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks

What if one path is undefined and the other is defined, wouldn't it wrongly return true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function only makes sense if a path exists as a field, otherwise it doesn't matter what it returns. If path is missing this function shouldn't be called at all, which happens when Navbar has a drop-down which holds a list of entries instead of a path.

With this quick fix I made it check for undefined and return arbitrary value further to hope the underlying logic works and with "true" it does work, probably with false as well as there's just no path.

The real fix should be somewhere in the navbar generation and there's obviously no test for a navbar with a drop-down which would catch this.

In 2.0.0-alpha.65 it still works fine. With .66 it breaks. Removing a drop-down from docusaurus config makes it work with with .66 as well.

The fix I propose is doing nothing bad, it simply does the check only if none of the values are undefined and returns smth to avoid 'undefined' further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's more of a workaround. I'll see if I can fix it in the reverting of the Navbar

};