-
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
Added CSP executeScript fallback #80
Conversation
src/content/init.js
Outdated
}).json; | ||
const json = await data; | ||
|
||
window.__discoveryPreloader = preloader; // eslint-disable-line no-underscore-dangle |
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 suppose there is no need to expose preloader, since we just do preloader.el.remove()
we can do it right before initDiscovery check
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.
We have to wait for the discovery initialization. In the case of the fallback, this happens in a separate context, so I pass it through global. Tested it on large JSONs
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.
Well, preloader is needed while data is loading. Is it actual when data is loaded?
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.
Sure. Otherwise the loading will stop at data parsing step and you will see the Discovery immediately (skipping the init ui, prepare etc steps)
src/content/init.js
Outdated
if (typeof initDiscovery !== 'function') { | ||
await chrome.runtime.sendMessage({ type: 'initDiscovery' }); | ||
} else { | ||
await initDiscovery(...window.__discoveryOptions); |
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.
Suggestion:
const discoveryOptions = {...};
if (typeof initDiscovery !== 'function') {
window.__discoveryOptions = discoveryOptions;
chrome.runtime.sendMessage({ type: 'initDiscovery' });
} else {
initDiscovery(...discoveryOptions);
}
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.
Fixed
src/content/init.js
Outdated
|
||
preloader.el.remove(); | ||
const { initDiscovery } = await import(chrome.runtime.getURL('discovery-esm.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 believe you should try revert to Promise.all()
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.
Reverted
preloader.el.remove(); | ||
const { initDiscovery } = await import(chrome.runtime.getURL('discovery-esm.js')); | ||
|
||
if (typeof initDiscovery !== 'function') { |
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.
Would be nice to add a comment what is going on here
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.
Added comment
Fixes #78