[Bug]: The deprecation of npm hook means that the site no longer supports the letter H

laine-hallot commented 2 weeks ago

What??

The issue title is a bit of a joke but its just describing a funny end result of what was probably a minor oversight when generating the docs for v10.

This started when I tried to find info on npm scripts that run after specific npm commands like "postinstall": "". Capacitor calls them hooks (maybe some other projects do too idk) so I figured I'd search the "Hooks" in the search box, but as soon as I typed the first letter "H" this happened

npm-hates-the-letter-H

The site completely crashed immediately after I typed "H" ("h" also breaks). I can get this to happen every time in both Chrome and Firefox and no other query does this (not exactly true "npm-h" also breaks it).

I pulled the code for the site and tested it locally and turns out the issue is that search finds the article at /cli/v10/commands/npm-hook (https://github.com/npm/documentation/blob/main/content/cli/v10/commands/npm-hook.mdx) but when <SearchResults/> tries to match its path to bread crumbs it gets an empty array and then tries reading a negative index.

It gets no breadcrumb results since there's no entry for npm-hook under v10 in content/nav.yml https://github.com/npm/documentation/blob/f4898d9d18e42eeb6da40393cf64227c5d37e645/content/nav.yml#L1404-L1408 which I'm assuming is related to the fact that the hooks api was deprecated - https://github.blog/changelog/2024-07-16-sunset-notice-npm-hooks-api-endpoints/

Fix

I was able to fix the issue with three different approaches:

1. Don't access invalid index

This is just adding a check in src/components/search.js to see if hierarchy is empty which sort of fixes the issue but for some reason if you select the result for npm-hook the version dropdown on the doc page for it ends up being empty.

2. Delete the page

Deleting https://github.com/npm/documentation/blob/main/content/cli/v10/commands/npm-hook.mdx fixes the issue too but that only make sense if that page really wasn't intended to exist in the first place. I'd assume thats the correct approach but it looks like npm hook is still a command on npm v10

3. Add a nav entry

This lets setNav.getItemBreadcrumbs(item.path) actually return some results so <SearchResults/> doesn't break but adding the nav entry probably only makes sense if the page cli/v10/commands/npm-hook was intended to exist.

I would just make a PR with a fix but I don't know if the page for npm-hook should even exist under v10 or not so idk which route to go with.

kenshanta commented 1 week ago

This issue tackles multiple concerns:

Command/file for npm-hooks (depreciated) on npm v10 => maintainers' call
Current - missing - alternative(s) for npm-hooks => maintainers' call
Handling crashes without removing any content => contributors' call

Do you think by providing a default value for hierarchy would do the trick? If so would you like to create the PR for it?

laine-hallot commented 3 days ago

This issue tackles multiple concerns:
1. Command/file for `npm-hooks` (depreciated) on npm `v10` => maintainers' call

2. Current - missing - alternative(s) for `npm-hooks` => maintainers' call

3. Handling crashes without removing any content => contributors' call
Do you think by providing a default value for hierarchy would do the trick? If so would you like to create the PR for it?

@kenshanta Thanks for the input, I went ahead and put up a PR to at least handle empty hierarchy and stop the query from crashing the page. https://github.com/npm/documentation/pull/1351

npm / documentation