-
Notifications
You must be signed in to change notification settings - Fork 1
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
[Proposal] Feature: Keyword search capability for systems #190
Conversation
🦋 Changeset detectedLatest commit: 6569a01 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 Author commentary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 Most of these changes it seems were caused by auto formatting. Only real change is the addition of the getSearchKeywords
documentation
@@ -8,6 +8,9 @@ const isDemo = process.env.DEMO === 'true'; | |||
const productionCode = ` | |||
const mockTraces = []; | |||
export default mockTraces; | |||
export function generateLotsOfMockTraces() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 Not sure what this is all about, but it wouldn't build without it
await user.keyboard('{Shift>}'); | ||
await user.keyboard('{Meta>}'); | ||
await user.click(firstItem); | ||
await user.keyboard('{/Shift}'); | ||
await user.keyboard('{/Meta}'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 In the userEvent
library, {Key>}
means holding the key down, and {/Key}
means releasing it 🤷. Basically these five lines perform a click whilst holding (and then releasing) "Shift" and "Win/Cmd" keys.
@@ -582,7 +633,7 @@ describe('ApplicationContext', () => { | |||
}); | |||
|
|||
const valueAfter = getByTestId('value'); | |||
expect(valueAfter).toHaveTextContent('2'); | |||
expect(valueAfter).toHaveTextContent('1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 The change to how mock traces were organised meant that IDs were out of sequence, and subsequently meant that some tests were not exercising code fully. For example, a test which previously attempted to move to a previous trace wouldn't execute the code to go to the previous trace, because the currently selected trace had now become the first one.
A change in the mock data now ensures that traces will get sequential IDs, regardless of how they are organised.
@@ -4,7 +4,7 @@ import { elapseTime, requestData } from './util'; | |||
|
|||
// GQL query (people) | |||
const gqlQuery: Event = { | |||
id: '2', | |||
id: 'TBC', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 IDs in trace data now become irrelevant, because we rewrite the IDs to be sequential in the mockTraces/index.ts
file.
Should we consider a solution that is system-agnostic and uses the object keys (unfiltered or filtered) of incoming traces with a hash map or stringly based search? That could allow users to search on any value available without specifically updating the code. The stringly based search (where you concat fields into a single string) is less performant and higher likely hood of false positives, but simpler to implement. The hash map (where you map all incoming keys/value pairs into a map) is higher performance, but a little more difficult to implement. We could also consider a tag-based search approach where you can type the specific property into the search |
We can. That's certainly an option. Some considerations:
The tag-based approach is a good idea, and one which can work alongside the "generic" search. E.g., |
What does this PR do
Allows systems to expose keywords for traces, so that the search feature of Envy can find traces by more than just the URL of the trace.
Resolves #187
New
System<T>
interface member:System<T>
:getSearchKeywords
string[]
containing keywords they wish the main search to find a given trace by.GraphQL
system, where this allows searching for traces by the GQL operation name.CocktailDB
system, where this allows searching for traces by the name of the cocktail.customizing.md
README fileNew ability to inject a large volume of traces:
Screenshots
Using the search term to find CocktailDb API traces by cocktail name
How to test
Basic:
RegisterPerson
orPDP
Custom viewer:
yarn example:apollo:custom-viewer
apollo-client
on http://localhost:4001/www.thecocktaildb.com
API should be displayed