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

Support building reusable components: Single dependency passing #4266

Closed
wants to merge 10 commits into from

Conversation

vladsud
Copy link
Contributor

@vladsud vladsud commented Nov 6, 2020

This is draft PR, the purpose of it to demonstrate first steps on implementation of issue #4265 - Support building reusable components / eco-system, get overall feedback on design direction.
It will be broken into smaller PRs that can be easier reviewed.

There is a lot of glue code and many changes are simply massaging existing code to get it better shape to support functionality.
Here are the main things to pay attention:

  1. I'm reusing existing dependency management code, as it looks like good starting / staging point, but also is not really used anywhere
    • IFluidDependencySynthesizer is split, smaller IFluidDependencyProvider interface is added supporting only consumption flow.
  2. All the interesting logic is concentrated in DataObject & PureDataObjetFactory. The following changes are done:
    • Passing of dependencies is explicit, no more reliance on ContainerRuntime.scope.
    • Dependencies are automatically serialized and deserialized on load, assuming they support IFluidLoadable.
    • A method is added to wrap non-serializable object into serializable object, to simplify creation of input for child objects.
  3. UTs (packages/test/end-to-end-tests/src/test/dependency.spec.ts) can be reasonable proxy for usage, though it's a bit hard to follow code given structure of our UTs (some things are in core UT infra and usage flow is not very obvious).
  4. Better example of usage pattern is Pond & Vltava changes. They demonstrate passing passing around dependencies, and in case of Pond - wrapping non-serializable object into IFluidLoadable to be passed down to consumers.
  5. ContainerRuntimeFactoryWithDefaultDataStore is switch to take factory, not symbolic name.
    • if factory is DataObjectFactory, it will use direct (DataObjectFactory) methods of creating root component vs. using runtime
  6. Detached Root data store creation is added both to runtime & Aqueduct.

Next steps:

  1. Get it in over multiple PRs
  2. Use required argument semantics, not optional argument semantics when describing dependencies.
  3. Modify the system to support multiple dependencies. Current system only supports just one object, which is not sufficient.
  4. Move dependencies to be of IFludLodable type only.
  5. Have a better system to describe dependencies. It should be of more general form name: type, not type: type

@anthony-murphy
Copy link
Contributor

I haven't spent any time on this, but as a general note, i think aqueduct is getting pretty bloated and complex. i don't know if this is the right place/time, but we should consider pushing down concepts/patterns that make sesnse to the runtime layer, as ideally aqueduct is a light wrapper putting things together.

@vladsud
Copy link
Contributor Author

vladsud commented Nov 6, 2020

@anthony-murphy , thanks for feedback! If possible, I'd prefer to move here incrementally, as even these changes are rather massive (unfortunately), even though I've implemented only small part of what needs to be done here (i.e. it's mostly plumbing work). I'm reusing existing (but not used) functionality backed into DataObject, and for most part do not add that much new functionality. I'd prefer to keep going that route, and once it takes its reasonable shape (after all the "next steps" I mentioned above), it would be great time to re0evaluate.

What do you think?

Note that some things will become simpler for DataObject. For example, I think I'll combine optional & required templated args on DataObject,

@vladsud
Copy link
Contributor Author

vladsud commented Nov 10, 2020

I've raised 3 staging PRs from here. Once they settle, I'll raise final PR with actual changes.
Feedback is still welcome, but given I do not expect much traction till final PR will be raised, closing this one.

@vladsud vladsud closed this Nov 10, 2020
@vladsud vladsud deleted the Providers branch November 11, 2020 20:11
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.

2 participants