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

All modified and coverable lines are covered by tests ✅

Project coverage is 76.47%. Comparing base (4663859) to head (645c794).
Report is 27 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #888      +/-   ##
==========================================
+ Coverage   76.30%   76.47%   +0.17%     
==========================================
  Files          21       21              
  Lines         726      727       +1     
  Branches      136      136              
==========================================
+ Hits          554      556       +2     
  Misses        121      121              
+ Partials       51       50       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[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 [email protected]. 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.

[tsmaeder]

Hello, is this thing on? The PR has been open for 2.5 years and approved for half a year. Can we get a decision, please?

[erikian]

Looks like all feedback was addressed, so this should be good to land

[continuous-auth]

🎉 This PR is included in version 3.7.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀