load listOfInjectableURLRegex from news-sources.json #53
Conversation
|
What it does: Populates the listOfInjectableURLRegex with regex-form of the Urls from the local news-sources.json file. |
There was a problem hiding this comment.
Thanks @alexhan01!!! A couple of nits here and there but looks good to me! Tested it and it works!
| // Parses allowed news sources from news-sources.json | ||
| function parseAllowedNewsSource() { | ||
| // Fetches data from local .json file | ||
| fetch('../static/news-sources.json') |
There was a problem hiding this comment.
Is it possible to preserve the promise here?
Since this runs asynchrounosly, it would be nice to .then it later to make sure it's complete before we actually do any validation (as the array is empty initially, and is only populated once the promise is resolved)
| "^https:\\/\\/www\\.stackoverflow\\.com", | ||
| "^https:\\/\\/www\\.bbc\\.com\\/news\\/" | ||
| var listOfInjectableURLRegex = [ // change these to the list of allowed news sources | ||
| // "^https:\\/\\/www\\.google", |
There was a problem hiding this comment.
We can safely remove those comments
| var regexedUrl = new RegExp(rawUrl); | ||
| listOfInjectableURLRegex.push(regexedUrl); | ||
| } | ||
| // console.log(listOfInjectableURLRegex); // For Testing |
There was a problem hiding this comment.
Ditto on removing the comments
| function addToListOfInjectableURLRegex(json) { | ||
| const size = json.numRows; | ||
| for (i = 0; i < size; i++) { | ||
| var rawUrl = json.dataRows[i].baseUrl; |
There was a problem hiding this comment.
nit: Should be const not var
In modern javascript var is virtually never used. If you expect the variable to be re-assigned, you use let, otherwise you use const
| const size = json.numRows; | ||
| for (i = 0; i < size; i++) { | ||
| var rawUrl = json.dataRows[i].baseUrl; | ||
| var regexedUrl = new RegExp(rawUrl); |
| // Helper function for parseAllowedNewsSource | ||
| function addToListOfInjectableURLRegex(json) { | ||
| const size = json.numRows; | ||
| for (i = 0; i < size; i++) { |
There was a problem hiding this comment.
| for (i = 0; i < size; i++) { | |
| for (let i = 0; i < size; i++) { |
I would just use forEach though to avoid the indexing
|
So recently I was thinking it would probably be simpler to use just the content.js file and not the background.js file, since content.js is simpler and we probably don't need any script running in the background, in which case, it might make the most sense to have the regex checking in popup.js or content.js depending on how/where we want to use it (and delete the background.js file)? What do you guys think @alexhan01 @tarikeshaq? |
|
@MayKikuchi for the regex checking part - we would just need to run it everytime the user opens a new tab right? And for populating the listOfInjectableURLRegex array, we just need to do it once - I'm unsure at which point though (do we populate the array when a new tab is open? when the extension is installed? etc.). Regardless, for both cases above, I agree that we wouldn't need to constantly do regex-checking and array loading in the background. |
I would think so @tarikeshaq
I would think it might be easier to populate it each time the user opens the extension, otherwise if we only want to populate it once, we would need to store it somewhere (chrome storage?) and then check if the list exists in storage + fetch the list from storage anyways when we want to use it, wouldn't we? |
|
Sorry I took my time to respond! And yes - I agree, we should run it once only for new tabs. On how often to populate the array.... Honestly? if it makes it easier. Lets hard code them in the |
closes #51