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

Search boosts #1930

merged 29 commits into from May 30, 2022

Conversation

shmax
Copy link
Contributor

@shmax shmax commented May 12, 2022

Fix for #1929

Add some config options to enhance search behavior (mainly for large projects that produce a large result set when searching):

  • add search config to typedoc.json (todo: I don't know where to edit the schema definition)
  • add boost logic to Search.ts, so that user can increase relevance based on exactMatch, kind, or category
  • add numResults (I don't really need this one, I just thought it would be a good first exercise, but it is useful for troubleshooting)

Notes:

  1. category boosts don't currently work in the example app, as I couldn't figure out how to switch on the category plugin... please advise
  2. the search config is a big crazy object, whereas all the config options tend to be scalar or array types, presumably so that they can also be set via command line; not sure what to do about that
  3. I think @Gerrit0 wants some or possibly all of this to be on by default, which would certainly simplify above problems.

Great library, thanks so much.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented May 15, 2022

todo: I don't know where to edit the schema definition

This gets automatically generated by scripts/generate_options_schema.js when a new version is published.

category boosts don't currently work in the example app, as I couldn't figure out how to switch on the category plugin... please advise

The category plugin only does things if the @category tag is used in the project, it looks like the example project doesn't use it right now.

tsconfig.json Outdated
"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.

id: rows.length,
kind: reflection.kind,
name: reflection.name,
url: reflection.url,
classes: reflection.cssClasses,
classes: reflection.cssClasses ?? "",
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.

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

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.

@shmax
Copy link
Contributor Author

shmax commented May 16, 2022

@Gerrit0 ready for another look when you have time.

@Dayday10
Copy link

Very good Now get in there and make it happen I'm counting on you and thanks

@Gerrit0
Copy link
Collaborator

Gerrit0 commented May 17, 2022

I'm liking the look of this! Things I want to look at when I have more than a few minutes:

  • Can we pre-compute the group boost too, and just ship a "boost" field in the frontend?
  • Option validation functions for both searchCategoryBoosts and searchGroupBoosts to prevent someone doing something dumb like { asdf: [] }

@shmax
Copy link
Contributor Author

shmax commented May 17, 2022

Can we pre-compute the group boost too, and just ship a "boost" field in the frontend?
Option validation functions for both searchCategoryBoosts and searchGroupBoosts to prevent someone doing something dumb like { asdf: [] }

3601b26

Added validation for searchGroupBoosts, but I'm not sure how to validate the category names, as they're not known until the reflection is complete.

"Model": 1.2
},
"searchGroupBoosts": {
"class": 1.5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I'm lowercasing everything, but the actual kind is "Class". Any preference on casing?

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 removed all the toLowerCase stuff, so that the kind names align with requiredToBeDocumented. d6e5b18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Speaking of which, are you sure that searchKindBoosts wouldn't be a better name for this? You refer to "kind" in the description for requiredToBeDocumented...

Copy link
Collaborator

Choose a reason for hiding this comment

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

With 0.22, yes, searchKindBoosts does look like the better name - I'm looking ahead to 0.23 though, where TypeDoc accepts an @group tag. By default, groups are determined by kind, but this gives additional flexibility. I think I'm happy with this, just need to find the time to pull it down and play with it a bit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okie-doke

@shmax
Copy link
Contributor Author

shmax commented May 26, 2022

@Gerrit0 Sorry for being a pest, but is there any way we can get this merged? My team and I are sort of waiting on this, and if any problems arise I'm around to fix them--just say the word.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented May 26, 2022

I should be able to get this in on Monday -- life has been really busy the past couple weeks

@Gerrit0 Gerrit0 merged commit b39c602 into TypeStrong:master May 30, 2022
@shmax
Copy link
Contributor Author

shmax commented May 30, 2022

Hey, thanks for merging. 👍

I see you added tests and some extra bits of business; much obliged, I'll take it out for a spin tomorrow.

@shmax shmax deleted the search-boosts branch May 30, 2022 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants