#6690: split up brick registry and runtime dependency cycle

[twschiller]

What does this PR do?

• Part of Untangle known dependency cycles #6690
• De-couples the brick registry and the runtime
• I took two different approaches
  • For UserDefinedBrick, I passed the registry into the constructor
  • For ReducePipeline, I'm using a module-level variable that's initialized by the contentScript/testing framework

Remaining Work

• Determine why tests are failing due the new injectRegistries file for testing: https://stackoverflow.com/questions/70007464/jest-worker-encountered-4-child-process-exceptions-exceeding-retry-limit

Discussion

• Introducing a class + constructor for ReducePipeline would be a pain. We'd need to start passing a dependency/registry context around nearly every
• Because the brick registry is a singleton instance, injecting the dependency into the reducePipeline module seemed like the most straightforward approach
• Splitting up the cycle does not appear to have an impact on bundle size
• Introducing injectRegistries in the jest setup required me to adjust a few test's mocks/assumptions

Future Work

• Decouple starter bricks and other bricks: #6690: split up brick registry and runtime dependency cycle #6774 (comment)

Checklist

• Add tests: N/A
• New files added to src/tsconfig.strictNullChecks.json (if possible): N/A
• Designate a primary reviewer: @grahamlangford

[codecov]

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (953113c) 70.05% compared to head (10af467) 70.04%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6774      +/-   ##
==========================================
- Coverage   70.05%   70.04%   -0.01%     
==========================================
  Files        1188     1188              
  Lines       36936    36954      +18     
  Branches     6937     6939       +2     
==========================================
+ Hits        25875    25884       +9     
- Misses      11061    11070       +9     

Files	Coverage Δ
src/bricks/registry.ts	96.77% <100.00%> (-0.20%)	⬇️
src/testUtils/loggingMock.ts	100.00% <100.00%> (ø)
src/contentScript/contentScriptCore.ts	0.00% <0.00%> (ø)
src/registry/memoryRegistry.ts	87.09% <71.42%> (-1.04%)	⬇️
src/bricks/transformers/brickFactory.ts	77.31% <75.00%> (+0.38%)	⬆️
src/registry/internal.ts	58.24% <40.00%> (ø)
src/runtime/reducePipeline.ts	84.80% <72.72%> (-0.62%)	⬇️

... and 2 files with indirect coverage changes

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

[fregante]

Does this also resolve #4896?

[twschiller]

Does this also resolve #4896?

Good point, no it doesn't because the starter bricks are still strongly coupled because of the selectAllBlocks method:

pixiebrix-extension/src/bricks/util.ts

Line 68 in 10af467

export async function selectAllBlocks(

And also the mergeReaders method:

pixiebrix-extension/src/bricks/readers/readerUtils.ts

Line 45 in 10af467

export async function mergeReaders(

I think the coupling between starter bricks and bricks is less important because IIRC it doesn't introduce a dependency cycle. There's not bricks that reference a starter brick file

[github-actions]

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.