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

Update librarie.min.js & decode search text #8776

Merged
merged 1 commit into from
Apr 23, 2018
Merged

Conversation

alfarok
Copy link
Contributor

@alfarok alfarok commented Apr 19, 2018

Purpose

QNTM-4042

Supplemental Librarie.js PR

This PR enables all characters and spaces in search and encodes the string passed from librarie.js to Dynamo where the string is decoded and run through search. Previously, spaces were replaced in searches because the results were not accurately returned due to the string formatting. In a later PR all characters that were not a-z 0-9 were removed from the query string to prevent special characters from crashing librarie. This PR should evade those issues.

Looking forward we may want to consider modifying the search timeout logic to improve response fluidity.

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner

FYIs

@Racel @jnealb @smangarole

@alfarok alfarok changed the title update library.min.js & unescape search text Update librarie.min.js & unescape search text Apr 19, 2018
@@ -40,6 +40,8 @@ public override Stream GetResource(IRequest request, out string extension)
var pathAndQuery = uri.PathAndQuery;
var index = url.IndexOf(serviceIdentifier);
var text = url.Substring(index + serviceIdentifier.Length + 1);
// unescape query before searching
text = Uri.UnescapeDataString(text);
Copy link
Collaborator

Choose a reason for hiding this comment

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

so, this accepts all special chars?

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 believe so, here we unescape the encoded URI sent from Librarie.js before searching

@alfarok alfarok changed the title Update librarie.min.js & unescape search text Update librarie.min.js & decode search text Apr 19, 2018
@QilongTang
Copy link
Contributor

@alfarok Can you talk a bit about where we are after this PR, do we have similar search results with 1.3.3? Or we need to do more work to achieve that

@alfarok
Copy link
Contributor Author

alfarok commented Apr 19, 2018

@QilongTang librarie.js seems to be very close, if not 1 to 1 with in-canvas results now. This doesn't help the lag or performance. We may want to take a closer look out how the timeout was implemented which creates a delay in displaying the results (much worse in D4R). I don't think it is very efficient and may cause search to appear slower than it actually is. @mjkkirschner had some good ideas around improving this logic.

image

image

@mjkkirschner
Copy link
Member

@alfarok please run the library tests locally I do not know if @smangarole was able to add them to the self serve CI yet.

@alfarok
Copy link
Contributor Author

alfarok commented Apr 19, 2018

image

image

@mjkkirschner
Copy link
Member

LGTM on these.

@mjkkirschner
Copy link
Member

@alfarok I think creating a version of search that only runs after the search interaction is idle would increase performance a ton - also whenever search gets invoked, we go back and forth between c# and javascript over IPC, then we go back and forth again via an event, then log to analytics ...

@alfarok
Copy link
Contributor Author

alfarok commented Apr 20, 2018

image

@alfarok alfarok merged commit a5a45b6 into DynamoDS:master Apr 23, 2018
alfarok added a commit to alfarok/Dynamo that referenced this pull request May 4, 2018
QilongTang pushed a commit that referenced this pull request May 4, 2018
alfarok added a commit to alfarok/Dynamo that referenced this pull request May 7, 2018
alfarok added a commit that referenced this pull request May 7, 2018
[cherry-pick #8776] Update librarie.min.js & decode search text
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.

4 participants