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

feat(core): derived context #288

Merged
merged 1 commit into from
Aug 4, 2020
Merged

feat(core): derived context #288

merged 1 commit into from
Aug 4, 2020

Conversation

JozefFlakus
Copy link
Member

@JozefFlakus JozefFlakus commented Jul 31, 2020

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

  • @marblejs/messaging - EventBus initializes the reader with the copy of derived context, eg. HttpServer context. Marble.js dependency injection container (aka Context) is built around JS Map. When removing a token from the context, references are changed because the whole Map is cloned.
// the problem 👇 - new Context copy
const derivedContext = deleteAt(setoidContextToken)(SomeToken)(rootContext);

const context = await constructContext(derivedContext)(
  bindTo(FooToken)(Foo),
  bindTo(BarToken)(Bar),
);

What is the new behavior?

  • @marblejs/messaging - EventBus uses a new concept of derived context (aka Context in Context)
const context = await contextFactory(
  bindEagerlyTo(DerivedContextToken)(() => derivedContext),
  bindTo(FooToken)(Foo),
  bindTo(BarToken)(Bar),
);

How does it work?

  • A Reader registers a new dependency under DerivedContext token which points to a derived context object
  • A Reader doesn't have to delete tokens which points to itself (to avoid circular context resolving) because it doesn't have to resolve it again - parent will do it automatically
  • A Reader registers its own context that doesn't influence/mutate the derived parent context
  • When asking for a dependency the server/listener first tries to lookup it in his own Context, if not found it will try to look for a registered DerivedContext and recursively invoke the lookup process again for that context.
/**
 * Lookup the dependency for a token in a `Context`
 * @since v2.0.0
 */
export const lookup = (context: Context) => <T>(token: ContextToken<T>): O.Option<T> =>
  pipe(
    M.lookup(ordContextToken)(token, context),
    O.map(dependency => {
      if (!dependency.eval) return dependency;

      const boostrapedDependency = isLazyDependency(dependency)
        ? dependency.eval()(context)
        : dependency.eval(context);

      context.set(token, boostrapedDependency);

      return boostrapedDependency;
    }),
    O.fold(
      () => pipe(
        M.lookup(ordContextToken)(DerivedContextToken, context),
        O.chain((derivedContext: Context) => lookup(derivedContext)(token))),
      O.some),
  );
  • The concept can be shown as follows:
Context:
  |- [DerivedContextToken]: DerivedContext
  |                            |- [DerivedDependencyToken_1]: DerivedDependency_1
  |                            |- [DerivedDependencyToken_2]: DerivedDependency_2
  |             
  |- [FooToken]: Foo
  |- [BarToken]: Bar

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@JozefFlakus JozefFlakus added enhancement New feature or request scope: core Relates to @marblejs/core package labels Jul 31, 2020
@JozefFlakus JozefFlakus added this to the 3.4 milestone Jul 31, 2020
@JozefFlakus JozefFlakus self-assigned this Jul 31, 2020
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #288 into master will decrease coverage by 0.04%.
The diff coverage is 95.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
- Coverage   95.41%   95.37%   -0.05%     
==========================================
  Files         157      162       +5     
  Lines        2769     2981     +212     
  Branches      366      401      +35     
==========================================
+ Hits         2642     2843     +201     
- Misses        123      134      +11     
  Partials        4        4              
Impacted Files Coverage Δ
packages/core/src/context/context.hook.ts 100.00% <ø> (ø)
...ackages/core/src/effects/effectsContext.factory.ts 100.00% <ø> (ø)
...kages/core/src/http/server/http.server.listener.ts 78.26% <ø> (ø)
...ackages/core/src/http/server/http.server.tokens.ts 100.00% <ø> (ø)
packages/core/src/logger/logger.interface.ts 100.00% <ø> (ø)
packages/messaging/src/reply/reply.ts 100.00% <ø> (ø)
...rc/transport/strategies/amqp.strategy.interface.ts 81.81% <ø> (ø)
...ges/messaging/src/transport/transport.interface.ts 100.00% <ø> (ø)
.../testing/src/testBed/http/http.testBed.response.ts 75.00% <40.00%> (-13.24%) ⬇️
packages/core/src/+internal/utils/any.util.ts 76.92% <71.42%> (-6.42%) ⬇️
... and 50 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 5360e59...7e1a589. Read the comment docs.

Tomisiak
Tomisiak previously approved these changes Jul 31, 2020
@tstelzer
Copy link
Collaborator

tstelzer commented Aug 1, 2020

You so wonderfully documented the implications of these changes in the PR description, but I don't see any of that in the code. I'd love to see some of these less-obvious implications described at the code level.

@JozefFlakus
Copy link
Member Author

JozefFlakus commented Aug 1, 2020

You so wonderfully documented the implications of these changes in the PR description, but I don't see any of that in the code. I'd love to see some of these less-obvious implications described at the code level.

What do you mean? It only affects EventBus reader so far. Or do you mean the dedicated test case which covers this kind of scenario?

@tstelzer
Copy link
Collaborator

tstelzer commented Aug 1, 2020

You so wonderfully documented the implications of these changes in the PR description, but I don't see any of that in the code. I'd love to see some of these less-obvious implications described at the code level.

What do you mean? It only affects EventBus reader so far. Or do you mean the dedicated test case which covers this kind of scenario?

Well, for example, its not entirely obvious to me purely from code what DerivedContext means. The code is very clear in the sense that I understand what is done, but I don't understand the why 🤔

Lacking that knowledge its not entirely clear why https://github.com/marblejs/marble/pull/288/files#diff-2430a932986481cd11bfd4c84fc9dcebR105 doesn't lead to infinite recursion.

The PR at least hints at the purpose, but PR descriptions are lost in time. 🤷‍♂️

@JozefFlakus
Copy link
Member Author

JozefFlakus commented Aug 1, 2020

@tstelzer it is mostly an internal thing and I'm not sure if it can be documented differently... The documentation for DerivedContext will be added in docs.marblejs.com and I don't like writing long documentation sentences in the source code if it will be repeated in other place... GitHub PR history is also a place for discussing and summarizing design decisions. It is another point of documentation. If someone will be interested in this topic, he can find it here.

@tstelzer
Copy link
Collaborator

tstelzer commented Aug 1, 2020

@tstelzer it is mostly an internal thing and I'm not sure if it can be documented differently... The documentation for DerivedContext will be added in docs.marblejs.com and I don't like writing long documentation sentences in the source code if it will be repeated in other place... GitHub PR history is also a place for discussing and summarizing design decisions. It is another point of documentation. If someone will be interested in this topic, he can find it here.

👍

I like to document these kind of decisions on the code level, rather than in PRs or even commit messages. Different styles, I guess.

@JozefFlakus JozefFlakus merged commit 2210d85 into master Aug 4, 2020
@JozefFlakus JozefFlakus deleted the feat/derived-context branch August 4, 2020 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request scope: core Relates to @marblejs/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants