-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: create events proxy to message from webapp to contentScript #20
feat: create events proxy to message from webapp to contentScript #20
Conversation
Suggestions for better names for files, functions, variables, different locations, organization, and architecture ARE WELCOME. |
…returned-from-the-background-service
ef443b0
to
e0f782a
Compare
src/content/content-events.js
Outdated
let isWebappReady = false; | ||
export const waitForWebappReady = () => { | ||
return new Promise((resolve) => { | ||
const interval = setInterval(() => { | ||
if (isWebappReady) { | ||
clearInterval(interval); | ||
resolve(true); | ||
} | ||
}, 100); | ||
|
||
setTimeout(() => { | ||
clearInterval(interval); | ||
resolve(false); | ||
}, 5000); | ||
}); | ||
}; |
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.
IMO no need to resolve(false)
if you can call reject()
(built-in in promise)
suggesting the following implementation with setTimeout
let isWebappReady = false; | |
export const waitForWebappReady = () => { | |
return new Promise((resolve) => { | |
const interval = setInterval(() => { | |
if (isWebappReady) { | |
clearInterval(interval); | |
resolve(true); | |
} | |
}, 100); | |
setTimeout(() => { | |
clearInterval(interval); | |
resolve(false); | |
}, 5000); | |
}); | |
}; | |
let isWebappReady = false; | |
function onAppLoaded(timeout = 5000, sleep = 100) { | |
let started = Date.now(); | |
return new Promise((resolve, reject) => { | |
const interval = () => { | |
if (isWebappReady) { | |
resolve(); | |
return; | |
} | |
let duration = Date.now() - started; | |
if(duration >= timeout){ | |
reject(`could not load app. timeout exceeded after ${timeout} milliseconds`) | |
return; | |
} | |
setTimeout(interval, sleep); | |
}; | |
interval(); | |
}); | |
} |
example: https://replit.com/@jossef/wait-for-resource#index.js
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.
I don't see a significant difference, these are two different methods of performing the same task.
Resolved anyway.
src/content.js
Outdated
const isReady = await events.waitForWebappReady(); | ||
if (!isReady) { | ||
console.log('Webapp is not ready, aborting'); | ||
return; | ||
} |
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.
if resolved, assuming loaded. if rejected, assuming timeout. I would change implementation like so:
const isReady = await events.waitForWebappReady(); | |
if (!isReady) { | |
console.log('Webapp is not ready, aborting'); | |
return; | |
} | |
try{ | |
await events.onAppLoaded(); | |
catch (e){ | |
console.error("failed to initialize content script", e); | |
return; | |
} |
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.
resolved
fixes: #19
The
finder
, the code that finds packages on the page, first waits for a message from the injected script (custom-element.js
) to know it loaded, then for each package it:custom-element.js
)<indicator>
The injected script does this on loading:
The
Indicator.vue
component is reactive to the store.