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

add Offline Cores #5

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

add Offline Cores #5

wants to merge 7 commits into from

Conversation

allancoding
Copy link
Member

No description provided.

Copy link
Member

@ethanaobrien ethanaobrien left a comment

Choose a reason for hiding this comment

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

I can't tell what this PR changes because you changed the indentation space count. Indentation across the entire EmulatorJS project should always be 4 spaces, to keep consistency with code style.

@allancoding
Copy link
Member Author

I don't even know how that happened vscode must have auto formatted it.

@allancoding
Copy link
Member Author

I'm going to have to force push to fix it ug

Copy link
Member

@ethanaobrien ethanaobrien left a comment

Choose a reason for hiding this comment

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

Could you provide screenshots of any UI changes and explain the logic behind this PR? I've read through the code and think its just loading the list of cores to allow the user to select what is cached locally, but I want to confirm

sw.js Outdated Show resolved Hide resolved
sw.js Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@allancoding
Copy link
Member Author

image
image

index.html Outdated Show resolved Hide resolved
@ethanaobrien
Copy link
Member

EJS_disableDatabases should be disabled, or we should remove the ability to cache the cores. Otherwise both EmulatorJS and the demo page will be storing the core blobs

Comment on lines +354 to +358
urlsToCache.push(corePath + "reports/" + core.value + ".json");
urlsToCache.push(corePath + core.value + "-wasm.data");
urlsToCache.push(corePath + core.value + "-thread-wasm.data");
urlsToCache.push(corePath + core.value + "-legacy-wasm.data");
urlsToCache.push(corePath + core.value + "-thread-legacy-wasm.data");
Copy link
Member

Choose a reason for hiding this comment

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

There should be some sort of opt-in here. This is 4x the amount of needed storage to keep a core locally. A user isn't often going to be switching between these at all

index.html Show resolved Hide resolved
@@ -53,6 +81,10 @@ self.addEventListener("fetch", (event) => {
let url = (requestURL.hostname === "cdn.emulatorjs.org") ? event.request.url : requestURL.pathname;
const cache = await caches.open(CACHE_NAME);
if (requestURL.hostname === "cdn.emulatorjs.org" && !OFFLINE_FILES.includes(event.request.url)) {
const cachedResponse = await caches.match(event.request);
if (cachedResponse) {
Copy link
Member

Choose a reason for hiding this comment

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

At what point in the code is this cache updated?

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.

2 participants