Merge mozilla/dispensary into this project by willdurand · Pull Request #4006 · mozilla/addons-linter

willdurand · 2021-11-01T20:11:19Z

Fixes #869
Closes mozilla/dispensary#864
Closes #3601

First commit: This is an almost 1:1 import of mozilla/dispensary, with tests, docs and the CLI (scripts/dispensary). Except one test case in dispensary/test.index.js (for the cached libraries, rewritten using async/await and jest spyOn), only Prettier has been applied and paths have been adjusted. There is no other code change.

Second commit: Split the logic of the dispensary so that we have a "runtime" piece for addons-linter and another piece (updater) to update the dispensary hashes using scripts/dispensary.

Third commit: Simplify the dispensary updater to only do one thing: compute the hashes for the libraries in the json file, and output that to stdout.

Fourth commit: Removes request dep, replaced by node-fetch.

Fifth commit: Improves test coverage. Should be good now.

Last commit: add npm script to update the hashes.txt file

codecov · 2021-11-01T20:14:45Z

Codecov Report

Merging #4006 (17713d8) into master (05a2c14) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4006      +/-   ##
==========================================
+ Coverage   98.72%   98.73%   +0.01%     
==========================================
  Files          52       55       +3     
  Lines        2658     2683      +25     
  Branches      802      808       +6     
==========================================
+ Hits         2624     2649      +25     
  Misses         34       34

Impacted Files	Coverage Δ
src/linter.js	`99.15% <ø> (ø)`
src/dispensary/hasher.js	`100.00% <100.00%> (ø)`
src/dispensary/index.js	`100.00% <100.00%> (ø)`
src/dispensary/utils.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05a2c14...17713d8. Read the comment docs.

README.md

package.json

src/dispensary/index.js

src/main.js

package.json

fregante · 2021-11-01T22:09:21Z

On second thought, I think this is great and makes mozilla/dispensary#864 much easier. ~~In fact I think you can just move those dependencies to devDependencies instead and be done.~~ Edit: It looks like _getCachedHashes needs to be extracted too, which I can do separately.

The problem in that issue was how to deal with the change of package type on npm (from generator to data-only). This PR kills the dispensary package on npm so that issue no longer exists.

willdurand · 2021-11-02T08:36:23Z

Requesting pre-reviews to answer a few questions:

does that approach sound good?
do we want to update this patch to reduce its size (e.g., by dropping the command stuff in the Dispensary class)?

wagnerand · 2021-11-02T10:47:56Z

This looks good to me overall. I'm all for simplifying things, the command stuff was a bit hard to read if you aren't familiar with the code. Let's also try and keep this low effort.

willdurand · 2021-11-02T12:24:02Z

This looks good to me overall. I'm all for simplifying things, the command stuff was a bit hard to read if you aren't familiar with the code. Let's also try and keep this low effort.

Thanks. Yeah, I am not sure we need to keep the existing CLI stuff. We might not even need yargs. We could just have a Updater class or something that reads the libraries.json and updates hashes.txt. This would be called by the new script and nothing more. This would allow us to move most of the dependencies as dev deps too.

willdurand · 2021-11-02T12:58:34Z

On second thought, I think this is great and makes mozilla/dispensary#864 much easier. ~~In fact I think you can just move those dependencies to devDependencies instead and be done.~~ Edit: It looks like _getCachedHashes needs to be extracted too, which I can do separately.

The problem in that issue was how to deal with the change of package type on npm (from generator to data-only). This PR kills the dispensary package on npm so that issue no longer exists.

The second commit should do what you're describing here.

wagnerand · 2021-11-02T13:14:42Z

This looks good to me overall. I'm all for simplifying things, the command stuff was a bit hard to read if you aren't familiar with the code. Let's also try and keep this low effort.

Thanks. Yeah, I am not sure we need to keep the existing CLI stuff. We might not even need yargs. We could just have a Updater class or something that reads the libraries.json and updates hashes.txt. This would be called by the new script and nothing more. This would allow us to move most of the dependencies as dev deps too.

Ah I see, that makes sense to me. There are only two commands, of which the update one is the most important, I think I never used the output one. Also, I don't recall ever using one of the arguments.

fregante

The second commit should do what you're describing here.

Yessss, LGTM. If merged, it would close:

willdurand · 2021-11-02T13:54:30Z

The third commit simplifies the dispensary "updater".

willdurand · 2021-11-02T15:13:53Z

The diff coverage is 95.77%.

I can probably write some more tests to reach 100%.

willdurand · 2021-11-02T17:17:11Z

@wagnerand @rpl this is now ready for a review. Each commit should be self-contained, which should help understand which changes have been made (the first one being almost 1:1 as mentioned in the PR description). The subsequent changes were actually useful to reduce the overall diff.

wagnerand

Looks great to me overall, got a few questions here and there.

Great job! 👏 🏆

src/dispensary/updater.js

tests/unit/dispensary/test.updater.js

willdurand · 2021-11-03T12:40:00Z

(rebased)

rpl

@willdurand Thank you so much for having looked into this, I'm looking forward to the reductions of the node_modules/ size in web-ext too as a side effect of this changes 🥳

This looks pretty great to me, I do have only two small changes to request described below (the one related to the list of urls in the README does not really matter that much, the other two are the ones I mainly care about, they should both require only some small tweaks to what is already in this PR).

README.md

src/dispensary/updater.js

src/dispensary/index.js

willdurand · 2021-11-08T21:32:59Z

I think I addressed all the comments (correctly). The command continues to work, tests pass, all other checks seem OK too.

r?

rpl

Lgtm. Thank you!

willdurand commented Nov 1, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

src/dispensary/index.js Outdated Show resolved Hide resolved

src/main.js Outdated Show resolved Hide resolved

willdurand commented Nov 1, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

willdurand requested review from rpl and wagnerand November 2, 2021 08:33

willdurand force-pushed the merge-dispensary branch from a1f71b7 to 30cb87c Compare November 2, 2021 12:57

willdurand force-pushed the merge-dispensary branch 2 times, most recently from 0591c80 to c0a2f9a Compare November 2, 2021 13:08

fregante approved these changes Nov 2, 2021

View reviewed changes

willdurand marked this pull request as ready for review November 2, 2021 17:13

wagnerand reviewed Nov 3, 2021

View reviewed changes

src/dispensary/updater.js Outdated Show resolved Hide resolved

tests/unit/dispensary/test.updater.js Show resolved Hide resolved

willdurand force-pushed the merge-dispensary branch 2 times, most recently from 85bedb0 to eab90bb Compare November 3, 2021 12:37

rpl requested changes Nov 8, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

src/dispensary/updater.js Outdated Show resolved Hide resolved

src/dispensary/index.js Show resolved Hide resolved

willdurand force-pushed the merge-dispensary branch from 1154859 to 9dd20ee Compare November 8, 2021 21:24

willdurand requested a review from rpl November 8, 2021 21:31

willdurand added 3 commits November 9, 2021 09:57

Merge mozilla/dispensary into this project

7948fc5

Split dispensary logic to avoid loading unecessary deps

e7e6c98

Simplify dispensary updater

347783a

willdurand added 6 commits November 9, 2021 09:58

Replace request with node-fetch

4af6c94

Add some more tests

3e419c1

Add npm script to update the hashes

84b2b96

fix indentation in readme file

f749e24

move updater to scripts/dispensary, add some notes about hashes.txt

8823aaa

remove empty file

17713d8

willdurand force-pushed the merge-dispensary branch from a02f0bf to 17713d8 Compare November 9, 2021 08:59

rpl approved these changes Nov 9, 2021

View reviewed changes

willdurand merged commit 819adb3 into master Nov 9, 2021

willdurand deleted the merge-dispensary branch November 9, 2021 11:10

Conversation

willdurand commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fregante commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willdurand commented Nov 2, 2021

Uh oh!

wagnerand commented Nov 2, 2021

Uh oh!

willdurand commented Nov 2, 2021

Uh oh!

willdurand commented Nov 2, 2021

Uh oh!

wagnerand commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante left a comment

Choose a reason for hiding this comment

Uh oh!

willdurand commented Nov 2, 2021

Uh oh!

willdurand commented Nov 2, 2021

Uh oh!

willdurand commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wagnerand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

willdurand commented Nov 3, 2021

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willdurand commented Nov 8, 2021

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

willdurand commented Nov 1, 2021 •

edited

Loading

codecov bot commented Nov 1, 2021 •

edited

Loading

fregante commented Nov 1, 2021 •

edited

Loading

wagnerand commented Nov 2, 2021 •

edited

Loading

willdurand commented Nov 2, 2021 •

edited

Loading