refactor: enable ban-types linting rule and fix violations

[BBosman]

Pull Request

📖 Description

Brings in the ban-types changes from #363 and fixes all related violations. I also included the (remaining) changes from #338, as they were partially overlapping and would just introduce merge conflicts.

As requested on Discord by @fkleuver I set the defaultSeverity to error. I explicitly marked completed-docs, cognitive-complexity and no-any as warning. That last one shouldn't be in there, but the remaining any's are above my proficiency level, so I'll leave those for somebody else to fix 😊, after which the exception can be undone.

🎫 Issues

Related to #249.
Follow up to the aborted #363 and #338.

👩‍💻 Reviewer Notes

A lot of typing changes all over the map as a result. Mostly reverse engineering A few could maybe be tightened up a bit more based on implementation knowledge that I don't (yet) have, so feel free to give suggestions. 😄

📑 Test Plan

Trust CI.

⏭ Next Steps

See #249. And moving more parts from #363 over.

Also we need to figure out why CircleCI has "invalid" linting issues so we can switch defaultSeverity back to error.

[codeclimate]

Code Climate has analyzed commit 2d7bad8 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 92.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 92.7% (0.0% change).

View more on Code Climate.

[codecov]

Codecov Report

Merging #366 into master will not change coverage.
The diff coverage is 93.79%.

@@           Coverage Diff           @@
##           master     #366   +/-   ##
=======================================
  Coverage   92.31%   92.31%           
=======================================
  Files         122      122           
  Lines       11034    11034           
  Branches     1967     1967           
=======================================
  Hits        10186    10186           
  Misses        848      848

Impacted Files	Coverage Δ
packages/router/src/parser.ts	93.1% <ø> (ø)	⬆️
packages/router/src/history-browser.ts	86.95% <ø> (ø)	⬆️
...ges/runtime/src/observation/collection-observer.ts	82.08% <ø> (ø)	⬆️
packages/runtime/src/observation.ts	100% <ø> (ø)	⬆️
packages/router/src/link-handler.ts	88.63% <ø> (ø)	⬆️
...s/runtime/src/observation/subscriber-collection.ts	90.82% <ø> (ø)	⬆️
...ackages/runtime/src/observation/target-observer.ts	93.75% <ø> (ø)	⬆️
...time-html/src/observation/select-value-observer.ts	92.74% <ø> (ø)	⬆️
...kages/runtime/src/observation/property-observer.ts	83.87% <ø> (ø)	⬆️
...e-html/src/observation/style-attribute-accessor.ts	79.54% <0%> (ø)	⬆️
... and 34 more

---

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 9f228e8...12340d3. Read the comment docs.

[fkleuver]

Excellent! Also a merge conflict hot-spot upcoming.. :)

[fkleuver]

@EisenbergEffect You're the mastermind behind DI, could you double check if @BBosman got the typings correct for dependencies / staticDependencies / dynamicDependencies in the Factory and Invoker? I would say they need to be of type "key", right? (which isn't really a type.. since that's essentially string | Constructable | InterfaceSymbol | Resolver etc etc.. but I'm not sure

[EisenbergEffect]

@BBosman Same as the other PR. Router PR made things go 💥 Do you mind fixing this up?

[EisenbergEffect]

Ok, other PR is now merged.

[fkleuver]

I think type of key and type of deps should just be unknown. It can be Constructable, InterfaceSymbol or Resolver which are useful types because they are generic, but it can also be string (in fact anything that's not falsey-ish or Object is a valid key). So a type can never reliably be inferred from it.

I "do it anyway" in the IServiceLocator interface's get method because it's just too useful for end users to not do it. But they still need to explicitly cast string-key resources. Inside DI there's therefore no real value to it imo.

The only reason I explicitly specified inject as an array of InterfaceSymbol at various components in the other PR is because they are specified explicitly by us, so we know that's what they are. We can't know this inside DI.

Same goes for the register contract. As @EisenbergEffect mentioned the other day, the existing constraints are just a pain as they require you to always export resources with static register or a merged declaration of IResourceType. I think good API documentation + good errors is the best thing we can do there. string | object could be another option though (meaning either string or a non-primitive)

[BBosman]

I rebased this on master.

• Rebasing triggered some new "generic" linting violations, which I fixed along the way.
• I included the (remaining) changes from refactor(kernel): DI typing #338, they were partially overlapping and would just introduce more merge conflicts.
• As requested on Discord by @fkleuver I set the defaultSeverity to error. I explicitly marked completed-docs, cognitive-complexity and no-any as warning. That last one shouldn't be in there, but the remaining any's are above my proficiency level, so I'll leave those for somebody else to fix 😊, after which the exception can be undone.
• Not all feedback on this PR and refactor(kernel): DI typing #338 has been incorporated, as there is still some discussion on what types should be used and where. Getting this PR in a reviewable state (which it now is) should make it possible for @EisenbergEffect to give some insight based on his DI knowledge.

No clue why linting fails on CircleCI. Those errors don't show up locally and making the suggested changes actually breaks it locally.
EDIT: They're not new, but the defaultSeverity error change now surfaces them. They were "ignored" before.
EDIT2: Reverted defaultSeverity back to warning for now as suggested by @fkleuver on Discord.

[fkleuver]

Solid improvements across the board, especially in the router. LGTM @EisenbergEffect

[EisenbergEffect]

@BBosman Did I mess this up again by merging the other lint PR?

[BBosman]

@EisenbergEffect No you didn't, I did. By avoiding ts files touched by this PR in the other I thought we wouldn't have conflicts, but I forgot about touching the package.json files in both. 😊

I just rebased and it's ready to go.

[jwx]

This is really helpful, @BBosman. Thanks for doing this.

[BBosman]

Made the changes requested by @jwx.

[codecov]

Codecov Report

Merging #366 into master will not change coverage.
The diff coverage is 93.79%.

@@           Coverage Diff           @@
##           master     #366   +/-   ##
=======================================
  Coverage   92.31%   92.31%           
=======================================
  Files         122      122           
  Lines       11034    11034           
  Branches     1967     1967           
=======================================
  Hits        10186    10186           
  Misses        848      848

Impacted Files	Coverage Δ
packages/router/src/parser.ts	93.1% <ø> (ø)	⬆️
packages/router/src/history-browser.ts	86.95% <ø> (ø)	⬆️
...ges/runtime/src/observation/collection-observer.ts	82.08% <ø> (ø)	⬆️
packages/runtime/src/observation.ts	100% <ø> (ø)	⬆️
packages/router/src/link-handler.ts	88.63% <ø> (ø)	⬆️
...s/runtime/src/observation/subscriber-collection.ts	90.82% <ø> (ø)	⬆️
...ackages/runtime/src/observation/target-observer.ts	93.75% <ø> (ø)	⬆️
...time-html/src/observation/select-value-observer.ts	92.74% <ø> (ø)	⬆️
...kages/runtime/src/observation/property-observer.ts	83.87% <ø> (ø)	⬆️
...e-html/src/observation/style-attribute-accessor.ts	79.54% <0%> (ø)	⬆️
... and 34 more

---

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 9f228e8...12340d3. Read the comment docs.

[codecov]

Codecov Report

Merging #366 into master will not change coverage.
The diff coverage is 93.79%.

@@           Coverage Diff           @@
##           master     #366   +/-   ##
=======================================
  Coverage   92.31%   92.31%           
=======================================
  Files         122      122           
  Lines       11034    11034           
  Branches     1967     1967           
=======================================
  Hits        10186    10186           
  Misses        848      848

Impacted Files	Coverage Δ
packages/router/src/parser.ts	93.1% <ø> (ø)	⬆️
packages/router/src/history-browser.ts	86.95% <ø> (ø)	⬆️
...ges/runtime/src/observation/collection-observer.ts	82.08% <ø> (ø)	⬆️
packages/runtime/src/observation.ts	100% <ø> (ø)	⬆️
packages/router/src/link-handler.ts	88.63% <ø> (ø)	⬆️
...s/runtime/src/observation/subscriber-collection.ts	90.82% <ø> (ø)	⬆️
...ackages/runtime/src/observation/target-observer.ts	93.75% <ø> (ø)	⬆️
...time-html/src/observation/select-value-observer.ts	92.74% <ø> (ø)	⬆️
...kages/runtime/src/observation/property-observer.ts	83.87% <ø> (ø)	⬆️
...e-html/src/observation/style-attribute-accessor.ts	79.54% <0%> (ø)	⬆️
... and 34 more

---

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 9f228e8...2d7bad8. Read the comment docs.

[BBosman]

Now that the tests are also linted, ban-types also triggers there. Took the liberty to add a last minute commit to fix those.

@EisenbergEffect Ready to go imho.

[fkleuver]

@EisenbergEffect Let's get this in

[fkleuver]

@BBosman Did a quick last pass but looks good. A nice follow up might be to give InterfaceSymbol the unknown type argument as a default, so the whole thing doesn't need to be written out in full everywhere.