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.

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

Search boosts #1930

Merged
merged 29 commits into from May 30, 2022
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion example/typedoc.json
Expand Up @@ -2,5 +2,17 @@
"$schema": "https://typedoc.org/schema.json",
"entryPoints": ["./src"],
"sort": ["source-order"],
"media": "media"
"media": "media",
"search": {
"numResults": 12,
"boosts": {
"exactMatch": 2,
"byKind": {
"class": 1.2
},
"byCategory": {
"Lang": 10
}
}
}
}
16 changes: 12 additions & 4 deletions src/lib/output/plugins/JavascriptIndexPlugin.ts
Expand Up @@ -5,12 +5,13 @@ import {
DeclarationReflection,
ProjectReflection,
ReflectionKind,
} from "../../models/reflections/index";
import { GroupPlugin } from "../../converter/plugins/GroupPlugin";
} from "../../models";
import { GroupPlugin } from "../../converter/plugins";
import { Component, RendererComponent } from "../components";
import { RendererEvent } from "../events";
import { writeFileSync } from "../../utils";
import { DefaultTheme } from "../themes/default/DefaultTheme";
import type { IDocument } from "../themes/default/assets/typedoc/components/Search";

/**
* A plugin that exports an index of the project to a javascript file.
Expand Down Expand Up @@ -63,12 +64,15 @@ export class JavascriptIndexPlugin extends RendererComponent {
parent = undefined;
}

const row: any = {
const row: IDocument = {
id: rows.length,
kind: reflection.kind,
name: reflection.name,
url: reflection.url,
classes: reflection.cssClasses,
classes: reflection.cssClasses ?? "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to leave this as is - again looking to avoid increasing the search.js file size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, gotcha. I think I was getting some kind of build complaint somewhere, but between this and the errors I was getting from the dom stuff, maybe I have something configured wrong. I'll try to narrow it down tomorrow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's because IDocument doesn't define this property as optional - it should be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I'll touch that up, thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

categories: (reflection.categories ?? []).map(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing this is going to greatly increase the size of the search data file, which for large projects is already pretty big... could we instead apply the category boosts in this plugin and add a boost property?

Doing it this way means that this module could also theoretically emit an event with the rows before building the index, allowing third party plugins to easily hook in and change boosts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also need to check that this works properly with categorizeByGroup set to both true and false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually wasn't able to get any kind of reaction out of cateogry stuff. How does one activate the category plugin in the example app? I didn't see any reference to it in in typedoc.json

Copy link
Collaborator

Choose a reason for hiding this comment

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

The category plugin is always active - but it will only do something if the documented members use @category tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see; I guess I didn't notice that. Would it be okay if I sprinkle a few in there to exercise some of this stuff?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's controlled by the categorizeByGroup option - reflection.groups has a ReflectionGroup[] which can contain categories

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've been experimenting with that; I tried setting it to both true and false and neither value ever seems to result in reflection.categories being anything other than undefined, here.

Copy link
Contributor Author

@shmax shmax May 15, 2022

Choose a reason for hiding this comment

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

If I add this to CategoryPlugin:170, it starts working as expected:

                child.categories = child.categories ?? [];
                child.categories.push(category);

(note: that does result in infinitely self-referential recursion, which is probably not a good thing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe this is where your suggestion to encode the boost score rather than the cats themselves could come into play--I could write the numbers in the Category plugin, and dodge the crazy recursion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(category) => category.title
),
};

if (parent) {
Expand Down Expand Up @@ -100,7 +104,11 @@ export class JavascriptIndexPlugin extends RendererComponent {
"assets",
"search.js"
);

const searchConfig = this.application.options.getValue("search");

const jsonData = JSON.stringify({
searchConfig,
kinds,
rows,
index,
Expand Down
78 changes: 69 additions & 9 deletions src/lib/output/themes/default/assets/typedoc/components/Search.ts
@@ -1,19 +1,23 @@
import { debounce } from "../utils/debounce";
import { Index } from "lunr";
import type { SearchConfig } from "../../../../../../utils/options/declaration";
import { ReflectionKind } from "../../../../../../models/reflections/kind";

interface IDocument {
export interface IDocument {
id: number;
kind: number;
name: string;
url: string;
classes: string;
parent?: string;
categories: Array<string>;
}

interface IData {
kinds: { [kind: number]: string };
rows: IDocument[];
index: object;
searchConfig: SearchConfig;
}

declare global {
Expand Down Expand Up @@ -78,19 +82,26 @@ export function initSearch() {
base: searchEl.dataset["base"] + "/",
};

bindEvents(searchEl, results, field, state);
bindEvents(
searchEl,
results,
field,
state,
window?.searchData?.searchConfig ?? {}
);
}

function bindEvents(
searchEl: HTMLElement,
results: HTMLElement,
field: HTMLInputElement,
state: SearchState
state: SearchState,
searchConfig: SearchConfig
) {
field.addEventListener(
"input",
debounce(() => {
updateResults(searchEl, results, field, state);
updateResults(searchEl, results, field, state, searchConfig);
}, 200)
);

Expand Down Expand Up @@ -140,7 +151,8 @@ function updateResults(
searchEl: HTMLElement,
results: HTMLElement,
query: HTMLInputElement,
state: SearchState
state: SearchState,
searchConfig: SearchConfig
) {
checkIndex(state, searchEl);
// Don't clear results if loading state is not ready,
Expand All @@ -154,9 +166,57 @@ function updateResults(
// Perform a wildcard search
// Set empty `res` to prevent getting random results with wildcard search
// when the `searchText` is empty.
const res = searchText ? state.index.search(`*${searchText}*`) : [];
let res = searchText ? state.index.search(`*${searchText}*`) : [];

if (searchConfig.boosts != undefined) {
for (let i = 0; i < res.length; i++) {
const item = res[i];
const row = state.data.rows[Number(item.ref)];
let boost = 1;

// boost by exact match on name
if (
searchConfig.boosts.exactMatch &&
row.name.toLowerCase() === searchText.toLowerCase()
) {
boost *= searchConfig.boosts.exactMatch;
}

// boost by kind
for (let kindName in searchConfig.boosts.byKind ?? {}) {
const kind: ReflectionKind = parseInt(
Object.keys(ReflectionKind).find(
(key: string) =>
ReflectionKind[key as keyof typeof ReflectionKind]
.toString()
.toLowerCase() === kindName.toLowerCase()
) ?? "",
10
);
if (row.kind == kind) {
boost *= searchConfig?.boosts?.byKind?.[kindName] ?? 1;
}
}

// boost by category
for (let categoryTitle in searchConfig.boosts?.byCategory ?? []) {
if (row.categories.indexOf(categoryTitle) > -1) {
boost *=
searchConfig.boosts.byCategory?.[categoryTitle] ?? 1;
}
}

item.score *= boost;
}

res.sort((a, b) => b.score - a.score);
}

for (let i = 0, c = Math.min(10, res.length); i < c; i++) {
for (
let i = 0, c = Math.min(searchConfig.numResults ?? 10, res.length);
i < c;
i++
) {
const row = state.data.rows[Number(res[i].ref)];

// Bold the matched part of the query in the search results
Expand Down Expand Up @@ -199,11 +259,11 @@ function setCurrentResult(results: HTMLElement, dir: number) {
// current with the arrow keys.
if (dir === 1) {
do {
rel = rel.nextElementSibling;
rel = rel.nextElementSibling ?? undefined;
} while (rel instanceof HTMLElement && rel.offsetParent == null);
} else {
do {
rel = rel.previousElementSibling;
rel = rel.previousElementSibling ?? undefined;
} while (rel instanceof HTMLElement && rel.offsetParent == null);
}

Expand Down
13 changes: 12 additions & 1 deletion src/lib/utils/options/declaration.ts
Expand Up @@ -3,7 +3,7 @@ import type { LogLevel } from "../loggers";
import type { SortStrategy } from "../sort";
import { isAbsolute, join, resolve } from "path";
import type { EntryPointStrategy } from "../entry-point";
import type { ReflectionKind } from "../../models/reflections/kind";
import { ReflectionKind } from "../../models/reflections/kind";

export const EmitStrategy = {
true: true, // Alias for both, for backwards compatibility until 0.23
Expand Down Expand Up @@ -50,6 +50,16 @@ export type TypeDocOptionValues = {
: TypeDocOptionMap[K][keyof TypeDocOptionMap[K]];
};

const Kinds = Object.values(ReflectionKind);
export interface SearchConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd like to split this up into a few options:

  • maxSearchResults
  • searchCategoryBoosts
  • searchGroupBoosts

I think it makes sense to always give exact matches a large boost, probably needs experimentation to figure out what that should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed exactMatch from the config stuff, and now it's on by default.

545fc88

Copy link
Contributor Author

Choose a reason for hiding this comment

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

8864deb

(removed the numResults thing; we don't need it right now, and I don't want to needlessly prolong this review)

numResults?: number;
boosts?: {
exactMatch?: number;
byKind?: { [key: typeof Kinds[number]]: number };
byCategory?: { [key: string]: number };
};
}

/**
* Describes all TypeDoc options. Used internally to provide better types when fetching options.
* External consumers should likely use {@link TypeDocOptions} instead.
Expand Down Expand Up @@ -107,6 +117,7 @@ export interface TypeDocOptionMap {
version: boolean;
showConfig: boolean;
plugin: string[];
search: unknown;
logger: unknown; // string | Function
logLevel: typeof LogLevel;
markedOptions: unknown;
Expand Down
5 changes: 5 additions & 0 deletions src/lib/utils/options/sources/typedoc.ts
Expand Up @@ -70,6 +70,11 @@ export function addTypeDocOptions(options: Pick<Options, "addDeclaration">) {
help: "Ignore protected variables and methods.",
type: ParameterType.Boolean,
});
options.addDeclaration({
name: "search",
help: "Configure search behavior",
type: ParameterType.Mixed,
});
options.addDeclaration({
name: "disableSources",
help: "Disable setting the source of a reflection when documenting it.",
Expand Down
17 changes: 7 additions & 10 deletions tsconfig.json
@@ -1,44 +1,41 @@
{
"compilerOptions": {
"module": "CommonJS",
"lib": ["es2019", "es2020.promise", "es2020.bigint", "es2020.string"],
"lib": [
"es2019",
"es2020.promise",
"es2020.bigint",
"es2020.string",
"dom"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't have this here. DOM stuff isn't available in Node, where we generally run, so it's a bad idea to have it for type checking. (This is why the index plugin didn't import IDocument before, I'm fine moving that so that it can be used by both projects)

Copy link
Contributor Author

@shmax shmax May 16, 2022

Choose a reason for hiding this comment

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

I'm still a bit confused about this. If I remove "dom" then I get build errors in Search.ts, which has references to dom concepts such as "document" and HTMLScriptElement. I assume this has something to do with my exporting IDocument out of Search.ts, but even when I put everything back the way it was I still get errors like this:

> esbuild src/lib/output/themes/default/assets/bootstrap.ts --bundle --minify --outfile=static/main.js

X [ERROR] Could not resolve "path"      

    src/lib/models/sources/file.ts:1:22:
      1 │ import * as Path from "path";
        ╵                       ~~~~~~

  The package "path" wasn't found on the file system but is built into node. Are you trying to
  bundle for node? You can use "--platform=node" to do that, which will remove this error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it has something to do with importing ReflectionKind into Search--it seems to open cross-ts-checking that is not meant to be across some kind of separation in the themes folder structure that I don't understand.

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the IDocument stuff. I'll leave it for someone else to figure out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That error is because the frontend doesn't have access to node modules, and bootstrap.ts was updated to import IDocument from a path which also ended up importing something that depended on native node modules. I'm fine just leaving the any for now, hasn't really caused any major pain. Still need to remove dom here though.

Copy link
Contributor Author

@shmax shmax May 17, 2022

Choose a reason for hiding this comment

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

Whoops, forgot to remove "dom". 26d4a9a

I have to say I found several aspects of this project a bit confusing:

  • the stuff in /themes seems to be silo'ed off into its own space, but the separation is sort of passive and not clear (If you really, really don't want it to mix with the library stuff, why not put it in a separate package, and have it import stuff from package scope like any other standalone app?
  • what, if anything, checks files like Search.ts ? You can type a garbage string right into the middle of the file, and build, build:tsc and build:themes all pass without complaint
  • some files under /themes do seem to reach up to get interfaces from the main library code (eg here)
  • the "lint" command doesn't actually report any errors, which is a bit maddening. I don't know if this is the way it's intended to work, but I have to think something is a bit off.

Etc. Overall, I enjoyed exploring the code.

],
"target": "es2019",

// Add our `ts` internal types
"typeRoots": ["node_modules/@types", "src/lib/types"],
"types": ["node", "glob", "lunr", "marked", "minimatch", "mocha"],

// Speed up dev compilation time
"incremental": true,
"tsBuildInfoFile": "node_modules/.cache/.tsbuildinfo",

"experimentalDecorators": true,

"strict": true,
"alwaysStrict": true,

// For tests
"resolveJsonModule": true,

// Linting
"noUnusedLocals": true,
"noUnusedParameters": true,
"forceConsistentCasingInFileNames": true,
"importsNotUsedAsValues": "error",

// Library
"preserveConstEnums": true,
"declaration": true,
"sourceMap": true,
"isolatedModules": true,
"noImplicitOverride": true,
"noPropertyAccessFromIndexSignature": true,

// Output
"outDir": "dist/",
"rootDir": "src/",
"newLine": "LF",

"jsx": "react",
"jsxFactory": "JSX.createElement",
"jsxFragmentFactory": "JSX.Fragment"
Expand Down