Modularize manifest files #94
Conversation
…removed with a force push before merging PR
There was a problem hiding this comment.
Good work here, Emil. I've left a few comments in the code, some of them petty and optional. Regarding your 3 bullet points in the PR description, I wish I could say authoritatively if the discrepancies are just inconsistent coding or if the nuances are actually merited.
What I would recommend is consolidating the manifest files as much as possible (the point of this PR, really!) while testing each environment very carefully. For example, in dev mode for Chrome, I'm now getting this console error, newly introduced in this PR.
Maybe the style-src content_security_setting isn't necessary in FireFox, but it is in Chrome, evidently. Can you go through Chrome in dev and prod mode, Firefox in dev and prod mode, and check for these errors, and heuristically determine the nuances?
Finding the balance of Don't Repeat Yourself while avoiding a centralized bloated mess full of conditionals and edge cases is always hard. Especially in something like a browser extension. I think this PR is striking a better balance than what we had but maybe we've gone a tad too far and need to add some of the specific things back. Or maybe add them to all environments, if that works! I'll let you decide.
| let jsonA; | ||
| let jsonB; | ||
| if ((typeof manifestTypeA) === 'string') { | ||
| jsonA = JSON.parse(fs.readFileSync(`${__dirname}/../chrome/manifest.${manifestTypeA}.json`)); |
There was a problem hiding this comment.
Is this better than just importing all the manifest.json files and then using the proper one here? This feels a little bit screwy to me.
There was a problem hiding this comment.
Yeah that might be nice, so as in always only pass in objects
| // Disabling eslint rule for code clarity, either key exists | ||
| // in both objects or it doesn't, 2 different cases with subcases | ||
| /* eslint-disable no-lonely-if */ | ||
| _.forEach(objA, (value, key) => { |
There was a problem hiding this comment.
We have a ton of looping functions in ES6. Could we use one of them over lodash? Or at a minimum could we only import the parts of lodash that we need (i.e. webpack tree shaking).
I know we've been using lodash throughout this plugin but it'd be nice to get away from it as much as possible.
There was a problem hiding this comment.
Okay sure. So you prefer we use Object.keys(obj).forEach((key) => {}) instead?
And yeah I think we could definitely get rid of lodash if we wanted, as I only noticed it being used very few times.
I'm used to using it with my other projects, but if you prefer not to that's very fine.
|
|
||
| // Deep copy both objects just to be safe | ||
| const objA = JSON.parse(JSON.stringify(passedObjA)); | ||
| const ret = JSON.parse(JSON.stringify(passedObjB)); |
There was a problem hiding this comment.
ret is a little cryptic. Maybe just me? Would something like output or returnedObject be better? I wasn't sure exactly that ret meant return without looking at the very bottom of the function. Webpack will minify this on production so readability over brevity is nice in the source code. Feel free to push back on this though.
There was a problem hiding this comment.
Yeah no problem, readability for sure. This is just a competitive programming habit haha ;). Everyone uses ret in that community.
| } | ||
| return mergeObjects(jsonA, jsonB, manifestTypeA, manifestTypeB); | ||
| } | ||
|
|
There was a problem hiding this comment.
Wow. This entire green block is some really deep code. It's complicated and troubleshooting any errors or extending it in the future will be intense. It seems that the majority of what this does over built-in functions like Object.assign() is that this outputs a ton of error messages. Do we really need all of this fanciness and complexity?
There was a problem hiding this comment.
I'll look into it, I might have gotten a bit carried away ;)
|
Yep, thanks for the review! I'll look into your comments and try and heuristic my way through finding a balance / correct manifest format. |
This resolves #84.
Note: I have a temporary final commit that I will delete and force push before merging, it contains the built manifests with their old names in order to be able to review what the scripts output will look like in this PR while reviewing changes.
My biggest thoughts are actually on the actual manifests more than the code. Like for example:
managementand only included one image size instead of the normal 3, while the production firefox manifest didn't have any of this (it didn't require management and had all 3 images). I'm assuming this is an oversight exactly because manifests weren't modularized?Yeah so the above questions would be awesome to have some dialogue on. The one I'm most confused about is the content_security_policy. I assume there shouldn't really be a difference between Firefox and Chrome there right? So shouldn't we try and come up with a common CSP that we can apply to all of them? If we can't I have added support in the script to merge CSPs though, so if needed we can also do that, but currently I know that what I have done is not correct anyway.