Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

💄 JS files lose all unused vars and imports #100

Merged
merged 14 commits into from
Dec 2, 2024

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Dec 2, 2024

This PR is where things get a little spicy. I am simply removing every var and import that is unused!
There are a few categories here:

  • The simplest is just tech debt, a var or function argument was defined (and probably used at some point) and later removed from he function body, but left in the body
  • vars that are defined for their side-effects (you can just remove them and call the methods alone)
  • unused package imports (self explanatory)

This PR affects the following plots:

  • pie_exploded
  • tech_exposure
  • tech_exposure_company
  • techmix_sector
  • timeline
  • trajectory_alignment

I will post side-by-sides of these plots before and after to show they still render (and I have visually inspected them to ensure all interactivity still functions as expected)

@MonikaFu FYI

Relates to #96
Towards #29

@jdhoffa jdhoffa requested a review from MonikaFu as a code owner December 2, 2024 15:58
@jdhoffa
Copy link
Member Author

jdhoffa commented Dec 2, 2024

pie_exploded

before

Screenshot 2024-12-02 at 17 03 55

after

Screenshot 2024-12-02 at 17 07 22

tech_exposure

before

Screenshot 2024-12-02 at 17 04 16

after

Screenshot 2024-12-02 at 17 07 40

tech_exposure_company

before

Screenshot 2024-12-02 at 17 04 39

after

Screenshot 2024-12-02 at 17 07 58

techmix_sector

before

Screenshot 2024-12-02 at 17 05 06

after

Screenshot 2024-12-02 at 17 08 23

timeline

before

Screenshot 2024-12-02 at 17 06 06

after

Screenshot 2024-12-02 at 17 08 44

trajectory_alignment

before

Screenshot 2024-12-02 at 17 05 41

after

Screenshot 2024-12-02 at 17 08 59

@jdhoffa jdhoffa merged commit 0add569 into main Dec 2, 2024
4 of 5 checks passed
@jdhoffa jdhoffa deleted the 29-linter-passes_unused_vars branch December 2, 2024 16:19
@jdhoffa jdhoffa mentioned this pull request Dec 2, 2024
@@ -1,5 +1,4 @@
import * as d3 from 'd3';
import { union } from 'd3-array';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

union is being used on line 130/129 no?

Copy link
Member Author

@jdhoffa jdhoffa Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, however that union call is a native method in the Set prototype.
Check it:

Adding the following console logging to the js there:

let subgroups = Array.from(subgroups0.union(subgroups1));
console.log(subgroups0.constructor.name);
console.log(Set.prototype.union);

and inspecting in the console, I see:
Screenshot 2024-12-02 at 19 19 14

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And note that if I add the line back:

import { union } from 'd3-array';

and do the same logging, it yields the same source:
Screenshot 2024-12-02 at 19 20 12

We were never calling the union method from d3-array in the first place

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be more backwards compatible to use the d3 implementation? assuming it does the same thing?

But I'm not sure how to even force it to actually do/ use that, and feels a bit out of scope for this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it must be a new-ish JS function. D3 used to provide stuff like that before they were available in vanilla JS. I guess luckily the syntax/interface/behavior is sufficiently similar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we can monitor it and if things start to seem off, we will know where to look :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect it is using a vanilla JS Set, and not an old-skool <=D3v4 set/map, but I don't really know the whole evolution of this code anymore, and it's spread out over multiple repos.

let subgroups0 = new Set(d3.map(subdataTechPerYear[0].stackedData, (d) => d.key).keys());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants