Extract search #303

mruwnik · 2023-08-20T18:18:56Z

Refactor the search module so it's independent (no external dependancies)
Extract the search module as a separate library
Add an example server+page to showcase that the independent search works by itself
Change the fronted to use the search library
Add CORS for selected routes (including static resources)
Allow searching directly from the url

This can also be seen in action here - might need to press enter for it to start searching. This involves hacky malusage of HTML, so is very clunky - I couldn't be bothered to do it properly.

A deployed version of this can be found here

mruwnik · 2023-08-20T18:20:42Z

app/components/search.tsx

@@ -107,12 +141,13 @@ export default function Search({onSiteAnswersRef, openQuestionTitles, onSelect}:
          </div>
        </AutoHeight>
      </div>
-      {showMore && (
+      {showMore && searcher && (


the searcher gets created as soon as the component is loaded. It's not properly configured then, as the worker still has to start up, but it's around, so this will be displayed as soon as the component gets loaded

mruwnik · 2023-08-20T18:21:42Z

app/routes/questions/search.tsx

+  return json(results, {
+    headers: {
+      'Access-Control-Allow-Methods': 'GET, OPTIONS',
+      'Access-Control-Allow-Origin': ALLOW_ORIGINS,


CORS stuff, so this can be called from other sites. It would probably be nicer as some kind of wrapper or something, but this should be fine for now?

mruwnik · 2023-08-20T18:23:13Z

server.js

+}
+
+const fetchCorsAsset = (event) => {
+  const resp = handleAsset(event, build)


the createEventHandler has no way to modify the resulting Response, nor any way to set CORS headers, so I ended up having to pretty much replicate some of the code paths in it :/

mruwnik · 2023-08-20T18:23:50Z

server.js

 import * as build from '@remix-run/dev/server-build'

-addEventListener('fetch', createEventHandler({build, mode: process.env.NODE_ENV}))
+const CORS_ASSETS = ['/tfWorker.js']


this is probably overkill. But if any other static resources need to be fetched with CORS, it should suffice to add them here

mruwnik · 2023-08-20T18:28:11Z

stampy-search/example/index.html

@@ -0,0 +1,62 @@
+<!DOCTYPE html>
+<html>


this is just a sample page to show how (that?) things work. I deployed it here

live questions don't work for me

mruwnik · 2023-08-20T18:30:51Z

stampy-search/src/search.ts

+
+const defaultSearchConfig = {
+  getAllQuestions: () => [] as Question[],
+  onResolveCallback: null,


used as a callback if a search succeeds and hasn't been canceled (e.g. by a new search being started)

mruwnik · 2023-08-20T18:31:17Z

stampy-search/src/search.ts

+    .trim()
+
+const defaultSearchConfig = {
+  getAllQuestions: () => [] as Question[],


this is called for the baseline search

mruwnik · 2023-08-20T18:32:44Z

stampy-search/src/search.ts

+      .then((response) => response.text())
+      .then((code) => {
+        const blob = new Blob([code])
+        return new Worker(URL.createObjectURL(blob))


CORS problems. The actual code can be fetched without any problems, but workers don't want to start if the js file is on a different domain. This is fine, though. I don't know what to think about how easy it is to get round these various js restrictions...

I don't believe using workers for shared modular code we expect other people to include on websites is a good idea 😢

mruwnik · 2023-08-20T18:35:13Z

stampy-search/src/search.ts

+      this.currentSearch.resolve(searchResults)
+      this.currentSearch = null
+      if (this.searchConfig.onResolveCallback) {
+        this.searchConfig.onResolveCallback(query, searchResults)


only call the callback if the result was for the most recent search. Cases like the following are fine:

search for "this is the first thing"

search for "this is the second one" -> will "cancel" the first one, but the worker will still be processing it

search for "this is the first thing" again -> will cancel the previous call, but if the worker returns the result from the first call, it will treat it as answering this one. Which is fine?

mruwnik · 2023-08-20T18:37:08Z

stampy-search/src/search.ts

+  runLiveSearch = (query: string, resultsNum?: number) => {
+    const questions = this.searchConfig.getAllQuestions ? this.searchConfig.getAllQuestions() : []
+    if (query != this.currentSearch?.query) {
+      return // this search has been superceeded with a newer one, so just give up


this can happen if a search is started before the worker is ready - it then waits 100ms and tries again. If a new query is started in the mean time, the first query shouldn't keep trying

Aprillion · 2023-08-22T10:00:36Z

stampy-search/README.md

This PR is too big for me to review right now, should get to it around August 30...

I noticed the deployed version does not update the search query in URL when updating search box.

Should it update the utl? I thought that this was already too big :/

No hurry on the reviewing. The main thing is that search can be invoked from other sites, and that's doable by e.g. importing https://media.ahiru.pl/stampy/stampySearch.min.js. I wanted this to be available before the aisafety.com hackathon thingy, in case it would be needed

URL is not getting any shorter by not updating it:

Feel free to completely remove the URL state if people don't need it. But if it's needed, it should be correct.

Aprillion · 2023-09-01T10:03:56Z

stampy-search/example/index.html

@@ -0,0 +1,62 @@
+<!DOCTYPE html>
+<html>


live questions don't work for me

Aprillion · 2023-09-01T10:14:08Z

stampy-search/src/search.ts

+      .then((response) => response.text())
+      .then((code) => {
+        const blob = new Blob([code])
+        return new Worker(URL.createObjectURL(blob))


I don't believe using workers for shared modular code we expect other people to include on websites is a good idea 😢

Aprillion · 2023-09-01T11:00:04Z

app/components/search.tsx

-    logSearch(value)
-  }
+      setPendingSearches(true)
+      searcher && searcher.searchLive(value)


this is probably mostly for TS reasons, but it shouldn't be a valid option to NOT perform the search at this step, if it "always works" it should throw an exception after future changes that make it suddenly "not work"

Aprillion · 2023-09-01T11:29:30Z

app/components/search.tsx

+  useEffect(() => {
+    setSearcher(
+      new Searcher({
+        getAllQuestions: () => onSiteAnswersRef.current,


This looks like a very useful exercise to find out how reusable is current architecture, and I'd be happy for this code to live on a branch or fork for experimental deployments.

However, I am not able to confirm this is the right step for the master branch and production deployment, where we should aim for more maintainability and simplified architecture. Adding more features on top of previous hacks does not sound sustainable to me.

For 3rd party users of this feature, creating new class instances to spin of workers sensitive to CORS and special API calls while having to provide your own getAllQuestions, then dealing with optional-results success callbacks without error handling and quite complex semantics ... that doesn't sound very simple to use / modular / ...

If there is someone else who wants to take responsibility for long term maintenance of this repo with occational firefighting and debugging if the site stops working, I'd be happy to hand over the "architecture oversight".

If not, I will rather focus on figuring out how to include stampy search on 3rd party sites using some simpler API, without this new PoC feature 🤔 Ideally something drop-in replacable with API from https://nlp.stampy.ai/ (or whatever is / will be the current url / name for that feature)

Aprillion · 2023-09-03T16:33:29Z

We are doing #307 at the aisafety.com hackathon and we found a compromise between practicality, magic, and at least halfway hoping it's still maintainable in PRs related to that issue => let's keep this branch for inspiration of future improvements, but not merging to master so closing the PR.

Thank you very much for exploring this path too!

mruwnik force-pushed the extract-search branch 2 times, most recently from 9b8a6c4 to b8759d9 Compare August 20, 2023 20:22

mruwnik commented Aug 20, 2023

View reviewed changes

mruwnik requested review from plexish, Aprillion and ccstan99 August 20, 2023 20:26

mruwnik force-pushed the extract-search branch from b8759d9 to 0e1a476 Compare August 20, 2023 20:31

Aprillion reviewed Aug 22, 2023

View reviewed changes

mruwnik added 4 commits August 22, 2023 12:46

Pure js search

3898f56

Separate stampy-search library

401aa88

unify search class

89cc4a1

Allow initial search in url

1383391

mruwnik force-pushed the extract-search branch from 0e1a476 to 1383391 Compare August 22, 2023 10:47

Aprillion reviewed Sep 1, 2023

View reviewed changes

Aprillion closed this Sep 3, 2023

Aprillion mentioned this pull request Sep 3, 2023

Support shareable url with search query (q and limit) #311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract search #303

Extract search #303

mruwnik commented Aug 20, 2023

mruwnik Aug 20, 2023

mruwnik Aug 20, 2023

mruwnik Aug 20, 2023

mruwnik Aug 20, 2023

mruwnik Aug 20, 2023

Aprillion Sep 1, 2023

mruwnik Aug 20, 2023

mruwnik Aug 20, 2023

mruwnik Aug 20, 2023

Aprillion Sep 1, 2023

mruwnik Aug 20, 2023

mruwnik Aug 20, 2023

Aprillion Aug 22, 2023

mruwnik Aug 22, 2023

Aprillion Aug 22, 2023

Aprillion Sep 1, 2023

Aprillion Sep 1, 2023

Aprillion Sep 1, 2023

Aprillion Sep 1, 2023

Aprillion commented Sep 3, 2023 •

edited

Loading

Extract search #303

Extract search #303

Conversation

mruwnik commented Aug 20, 2023

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

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

Choose a reason for hiding this comment

Aprillion commented Sep 3, 2023 • edited Loading

Aprillion commented Sep 3, 2023 •

edited

Loading