-
Notifications
You must be signed in to change notification settings - Fork 13
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
Manifest (performance improvement) #182
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* Returns the index of `needle' in `haystack', or `null' if it wasn't found. */ | ||
export function binarySearch(haystack, | ||
needle, | ||
cmp = (a, b) => a < b ? -1 : a > b ? 1 : 0) { | ||
let start = 0; | ||
let end = haystack.length - 1; | ||
|
||
while (start <= end) { | ||
const middle = Math.floor((start + end) / 2); | ||
const comparison = cmp(haystack[middle], needle); | ||
|
||
if (comparison == 0) | ||
return middle; | ||
else if (comparison < 0) | ||
start = middle + 1; | ||
else | ||
end = middle - 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get unit tests for this file and can you also convert it to TS? |
||
} | ||
|
||
return null; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,8 @@ | ||
import {AO_HOST} from "../client/aoHost" | ||
import {client} from "../client" | ||
import {canonicalizePath} from "./paths" | ||
import {binarySearch} from "./binarySearch" | ||
|
||
const fileExists = async (url) => new Promise((resolve, reject) => { | ||
const xhr = new XMLHttpRequest(); | ||
xhr.open('HEAD', url); | ||
|
@@ -16,3 +21,22 @@ const fileExists = async (url) => new Promise((resolve, reject) => { | |
xhr.send(null); | ||
}); | ||
export default fileExists; | ||
|
||
/* Returns whether file exists. | ||
* `url' is a URL, including the base URL for bw compat. | ||
* If manifest is empty, check the old way. | ||
* Otherwise, look it up in the manifest */ | ||
const fileExistsManifest = async (url) => | ||
new Promise((resolve, reject) => { | ||
if(client.manifest.length == 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
resolve(fileExists(url)); | ||
return; | ||
} | ||
const c_url = encodeURI(canonicalizePath(decodeURI(url.slice(AO_HOST.length)))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, JS uses camel case, so it may be a good idea to stick with that schema. Also calling it the full canonical. |
||
|
||
if(binarySearch(client.manifest, c_url) != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
resolve(true); | ||
|
||
resolve(false); | ||
}); | ||
export {fileExistsManifest}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import {client} from '../client' | ||
import {AO_HOST} from '../client/aoHost' | ||
import calculatorHandler from './calculatorHandler'; | ||
import fileExists from './fileExists.js'; | ||
import {fileExistsManifest} from './fileExists.js'; | ||
import { requestBuffer } from '../services/request.js'; | ||
/** | ||
* Gets animation length. If the animation cannot be found, it will | ||
|
@@ -11,7 +13,7 @@ const getAnimLength = async (url) => { | |
const extensions = ['.gif', '.webp', '.apng']; | ||
for (const extension of extensions) { | ||
const urlWithExtension = url + extension; | ||
const exists = await fileExists(urlWithExtension); | ||
const exists = await fileExistsManifest(urlWithExtension); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there's some extra spacing here |
||
if (exists) { | ||
const fileBuffer = await requestBuffer(urlWithExtension); | ||
const length = calculatorHandler[extension](fileBuffer); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,20 @@ | ||
export const getFilenameFromPath = (path: string) => path.substring(path.lastIndexOf('/') + 1) | ||
|
||
/* Removes `/./`, `/../`, and `//`. | ||
* Does not add a leading `/` or `./`. | ||
* Does not add a trailing `/`. */ | ||
export function canonicalizePath(path: string): string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure to update the unit tests please |
||
const path_elements = path.split("/"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. camelCasing here please |
||
var result: string[] = []; | ||
|
||
for (const el of path_elements) { | ||
if (el === "..") | ||
result.pop(); | ||
else if (el === "." || el === "") | ||
continue; | ||
|
||
result.push(el); | ||
} | ||
|
||
return result.join("/"); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import fileExists from './fileExists' | ||
import {fileExistsManifest} from './fileExists' | ||
import transparentPng from '../constants/transparentPng' | ||
const urlExtensionsToTry = [ | ||
'.png', | ||
|
@@ -10,11 +10,11 @@ const tryUrls = async (url: string) => { | |
for (let i = 0; i < urlExtensionsToTry.length; i++) { | ||
const extension = urlExtensionsToTry[i] | ||
const fullFileUrl = url + extension | ||
const exists = await fileExists(fullFileUrl); | ||
const exists = await fileExistsManifest(fullFileUrl); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unit tests for this please |
||
if (exists) { | ||
return fullFileUrl | ||
} | ||
} | ||
return transparentPng | ||
} | ||
export default tryUrls | ||
export default tryUrls |
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.
Make sure to update the unit tests for setEmote