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

Replace block executor registry with dependency injection #532

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

georg-schwarz
Copy link
Member

This PR solves an issue where the parallel execution of multiple Jayvee models led to side effects: an error by duplicate block executor registration.

Injecting the list of executors wrapped by a JayveeExecExtension to the ExecutionContext instead of the registry should solve that issue.

Additionally, the executors were registered for model parsing. I think this is not necessary anymore as their structure now comes from jv files instead, so we should only use executors when interpreting the model.

The changes led to a weird trace of dependency cycles that I still struggle with. Let me know if you see an easy way to resolve this :)

@georg-schwarz georg-schwarz changed the title Replace block executor registry by dependency injection Replace block executor registry with dependency injection Feb 16, 2024
@georg-schwarz georg-schwarz requested review from rhazn and joluj February 16, 2024 13:33
Copy link
Contributor

@joluj joluj left a comment

Choose a reason for hiding this comment

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

The circular dependencies can lead to problems. However, I could find a better way.

I've found these circular dependencies. Some of these were not introduced by this PR.

$ npx madge --circular --extensions ts ./
Processed 72 files (422ms) (25 warnings)

✖ Found 10 circular dependencies!

1) src/lib/debugging/debug-configuration.ts > src/lib/execution-context.ts
2) src/lib/blocks/block-executor-class.ts > src/lib/blocks/block-executor.ts > src/lib/debugging/debug-configuration.ts > src/lib/execution-context.ts > src/lib/extension.ts
3) src/lib/types/io-types/filesystem-node-file-binary.ts > src/lib/types/io-types/io-type-implementation.ts
4) src/lib/types/io-types/io-type-implementation.ts > src/lib/types/io-types/filesystem-node-file-text.ts
5) src/lib/types/io-types/io-type-implementation.ts > src/lib/types/io-types/filesystem.ts
6) src/lib/types/io-types/io-type-implementation.ts > src/lib/types/io-types/none.ts
7) src/lib/types/io-types/io-type-implementation.ts > src/lib/types/io-types/sheet.ts
8) src/lib/types/io-types/io-type-implementation.ts > src/lib/types/io-types/table.ts
9) src/lib/types/io-types/io-type-implementation.ts > src/lib/types/io-types/workbook.ts
10) src/lib/blocks/block-execution-util.ts > src/lib/blocks/block-executor-registry.ts > src/lib/blocks/composite-block-executor.ts

apps/interpreter/src/examples-smoke-test.spec.ts Outdated Show resolved Hide resolved
apps/interpreter/src/examples-smoke-test.spec.ts Outdated Show resolved Hide resolved
libs/execution/src/lib/blocks/block-executor-registry.ts Outdated Show resolved Hide resolved
@georg-schwarz georg-schwarz requested review from joluj and rhazn March 1, 2024 16:09
@rhazn rhazn force-pushed the execution-remove-block-executor-registry branch from b429e51 to 0cea216 Compare March 4, 2024 09:33
@rhazn rhazn merged commit f144c8c into main Mar 4, 2024
3 checks passed
@rhazn rhazn deleted the execution-remove-block-executor-registry branch March 4, 2024 09:42
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants