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

Implements search based on SurrealDB #24

Merged
merged 11 commits into from
Oct 20, 2023
Merged

Conversation

emmanuel-keller
Copy link
Contributor

@emmanuel-keller emmanuel-keller commented Oct 13, 2023

Implements the search backed by SurrealDB :)

git checkout emmanuel/surreal-search
pnpm run start

@netlify
Copy link

netlify bot commented Oct 13, 2023

Deploy Preview for surrealdb-docs ready!

Name Link
🔨 Latest commit 2597839
🔍 Latest deploy log https://app.netlify.com/sites/surrealdb-docs/deploys/653159f6d292f900086febf2
😎 Deploy Preview https://deploy-preview-24--surrealdb-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@emmanuel-keller emmanuel-keller temporarily deployed to github-pages October 13, 2023 16:11 — with GitHub Pages Inactive
@emmanuel-keller emmanuel-keller marked this pull request as ready for review October 13, 2023 16:13
@emmanuel-keller emmanuel-keller temporarily deployed to github-pages October 13, 2023 16:27 — with GitHub Pages Inactive
@emmanuel-keller emmanuel-keller temporarily deployed to github-pages October 13, 2023 16:31 — with GitHub Pages Inactive
@emmanuel-keller emmanuel-keller changed the title Emmanuel/surreal search Implements Search based on SurrealDB Oct 13, 2023
@emmanuel-keller emmanuel-keller changed the title Implements Search based on SurrealDB Implements search based on SurrealDB Oct 13, 2023
@emmanuel-keller emmanuel-keller temporarily deployed to github-pages October 13, 2023 17:11 — with GitHub Pages Inactive
@kearfy kearfy temporarily deployed to github-pages October 13, 2023 18:44 — with GitHub Pages Inactive
Use std color for the url
Keep the keywords when closing and reopening the search dialog
@emmanuel-keller emmanuel-keller temporarily deployed to github-pages October 13, 2023 20:49 — with GitHub Pages Inactive
}

function findBiggestOffsets(offsets) {
return offsets.map(({ s, e }) => e - s).reduce((a, b) => a + b, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using .map followed by .reduce, you can use a single .reduce to calculate the total length of offsets:

function findBiggestOffsets(offsets: Offset[]): number {
    return offsets.reduce((sum, { s, e }) => sum + (e - s), 0);
}

// - group offsets per line (see groupOffsets() function)
// - Sort descending based on the length of the offsets
// - pick the biggest offset
const [linenumber, offsets] = Object.entries(doc.offsets).sort(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of Object.entries(doc.offsets).sort(...)[0] to get the biggest offset, you could use a loop to find the largest without sorting, which would reduce the operation from O(n log n) to O(n). Sorting just to get the maximum value isn't efficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

let maxOffsetSize = -1;
let linenumber = null;
let offsets = null;

for (const [line, off] of Object.entries(doc.offsets)) {
    const currentOffsetSize = findBiggestOffsets(off);
    if (currentOffsetSize > maxOffsetSize) {
        maxOffsetSize = currentOffsetSize;
        linenumber = line;
        offsets = off;
    }
}

This above loop iterates over each entry of doc.offsets just once, computes the size of the offsets, and keeps track of the largest value seen so far. This is a direct O(n) operation, which is slightly more efficient than the O(n log n) operation from sorting, especially for large numbers of offsets.

};

async function query(sql: string) {
const raw = await fetch('https://blog-db.surrealdb.com/sql', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of interest, can we check that the server hosting 'https://blog-db.surrealdb.com/sql' is using compression like gzip for responses, which reduces the amount of data transmitted over the network?

Copy link
Contributor Author

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.

It is currently not the case. That would for sure be an great improvement

Screenshot 2023-10-19 at 14 15 11

Copy link
Contributor

Choose a reason for hiding this comment

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

Ref's Usage:
dialogRef is being used in the handleChange function, but it doesn't seem to be actually utilised. This makes it unnecessary to include it in the dependency array of the useCallback.
The same applies to the useEffect hook, which means the dialogRef dependency can be omitted entirely from both hooks.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's generally recommended to use the strict equality (===) instead of loose equality (==). In the conditional rendering section, you've used keywords == '', which should ideally be keywords === ''.

const [results, setResults] = useState<Doc[]>([]);

const handleChange = useCallback(async (value: string) => {
console.log('handleChange: ' + value);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove console.log

Copy link
Contributor Author

@emmanuel-keller emmanuel-keller Oct 19, 2023

Choose a reason for hiding this comment

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

Oops!

Copy link
Contributor

Choose a reason for hiding this comment

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

If the search API (search function) can be potentially slow or rate-limited, it would be a good idea to debounce the input changes so that not every keystroke triggers an API call.

function debounce(func, delay) {
  let inDebounce;

  return function(...args) {
    const context = this;
    clearTimeout(inDebounce);
    inDebounce = setTimeout(() => func.apply(context, args), delay);
  };
}
const debouncedSearch = useCallback(debounce(async (value) => {
    if (!value) {
        setResults([]);
        setKeywords('');
    } else if (value !== keywords) {
        setKeywords(value);
        try {
            let res = await search(value);
            console.log(res);
            setResults(res);
        } catch (err) {
            console.error(err);
        }
    }
}, 300), []); // Here, 300ms is the delay, we can re-adjust that if needed.

const handleChange = (value) => {
    debouncedSearch(value);
};

@emmanuel-keller emmanuel-keller temporarily deployed to github-pages October 19, 2023 13:09 — with GitHub Pages Inactive
@emmanuel-keller
Copy link
Contributor Author

Great review @dimitrianoudi! Everything's been applied

package.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks great, although it misses the Lodash dependancy.
Can you please do a:
pnpm install lodash
then commit the package.json and pnpm-lock.yaml and that should do!

@emmanuel-keller emmanuel-keller merged commit acb810c into main Oct 20, 2023
7 checks passed
@emmanuel-keller emmanuel-keller deleted the emmanuel/surreal-search branch October 20, 2023 08:26
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.

3 participants