fix: enforce extraModules to be a set of strings

[paul-marechal]

For some reason TypeScript is AOK with stuffing Set objects with
random keys. This is problematic as this is not how Set are supposed
to be used (like a plain JavaScript object map).

Fix the logic to respect the -w/--which-module CLI argument.

[codecov-commenter]

Codecov Report

Merging #888 (645c794) into main (4663859) will decrease coverage by 3.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #888      +/-   ##
==========================================
- Coverage   76.30%   73.17%   -3.14%     
==========================================
  Files          21       21              
  Lines         726      727       +1     
  Branches      136      136              
==========================================
- Hits          554      532      -22     
- Misses        121      146      +25     
+ Partials       51       49       -2     

Files	Coverage Δ
src/module-walker.ts	85.07% <100.00%> (+1.49%)	⬆️
src/rebuild.ts	73.11% <100.00%> (+0.29%)	⬆️

... and 3 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[paul-marechal]

@malept ping.

The -w/--which-module argument must have been broken for some time now, I have an issue where it doesn't work as far back as electron-rebuild@1.11.0. This patch rectifies the logic to make it work.

[ckerr]

Code LGTM. Approved, but I'll hold off merging since @malept was pinged in case he wants to review.

@paul-marechal please ping me if nothing happens in a week 😸

[paul-marechal]

@paul-marechal please ping me if nothing happens in a week 😸

@ckerr ping :)

[malept]

Can the added unit tests not cause modules to be rebuilt? Trying not to increase the test time when possible.

[paul-marechal]

@malept I am not familiar with how electron-rebuild works and how to do that... Do you have any pointers on how to test that --which-module/-w actually works without causing the rebuild? Is there a way to do some kind of dry run?

[paul-marechal]

@ckerr @malept updated the tests. I modified one of the tests to delete all dependencies from the package.json file and make use of extraModules.

[vince-fugnitto]

@malept @ckerr any updates?

[malept]

It seems to me like there should be a better way to test this. I need to think about it.

Also I'm not thrilled at the idea that a test has been modified, rather than adding a separate test.

[paul-marechal]

Can the added unit tests not cause modules to be rebuilt? Trying not to increase the test time when possible.

@malept I did it this way because of the above comment, I thought the new test I had added was causing issues...

edit: See the commit where I removed the extra tests.

With the current way, it won't build (many) more packages than what it used to before, keeping the test run time low. Moreover the specific test modified is specifically trying to rebuild packages that aren't part of dependencies, so this hackish way of modifying the package.json just for this case seemed reasonable to me.

If the test is that problematic I can always remove it :)

The previous lookup logic was broken without a doubt, and the fix in this PR is trivial.

[paul-marechal]

Your last commit looks good to me! Thanks for helping with this, I didn't feel familiar enough with the internals to do what you did.

[paul-marechal]

@malept should I do anything?

[vince-fugnitto]

@malept any update?

[vince-fugnitto]

ping.