TaskManager API docs cleanup

[skylerjokiel]

Since TaskManager is experimental and doesn't have a publicly exposed factory class I felt that it was a good candidate to do an initial run on. I don't think we will be able to be this aggressive with more widely used DDSes such as SharedMap.

One thing to note is that @internal does not prevent it being in intellisense or for the developer to find it. This is maybe okay but could be confusing. Intellisense does show the @internal marking.

Notes:

• constructor is not private. Can only create through create method or factory.
• All protected APIs are marked @internal to remove them from the API docs
• create docs are removed since create for 3P and internal differ
• TaskManager instance API matches ITaskManager

[tylerbutler]

One thing to note is that @internal does not prevent it being in intellisense or for the developer to find it.

Did you test this with "type trimming" on or off? It's harder to enable than I'd like, but I think it will exclude internal stuff from Intellisense. I have a draft change to enable type trimming for a package (I used counter) so I can point you to that if you want to try to do that too. We can do it separately too.

[ChumpChief]

I'm generally not a fan of this change. Marking things internal seems fine, but this is the wrong layer to commit to consumption-approach in and I really don't like hiding the constructor.

[ChumpChief]

I don't think we should remove this info, it's required for anyone not using the consumption-centric approach.

[tylerbutler]

Possible compromise for the near term -- mark one path as "beta?" It's somewhat disingenuous because they're both at roughly the same 'readiness level.'

[skylerjokiel]

This is a great comment and I'm not quite sure where I stand. On one point we are making statements that our public docs are specifically focused on 3P. On the other hand I agree that we can't simply ignore and steamroll 1P.

What I want to do is capture the nuanced scenarios that currently exist when it comes to creation. The hard part is that if our docs are 3P focused currently and we are sharing workflows that are not yet available we introduce confusion and complexity. I'm not sure how to balance these two.

[ChumpChief]

I really don't like this:

1. It's different from all the rest of our DDSs
2. We consistently get feedback that folks don't like our over-reliance on factories, this feels like a move in the wrong direction
3. Introduces a "real" dependency from TaskManager to TaskManagerFactory (current dependencies are just convenience static functions)
4. I'm personally averse to private constructors

[skylerjokiel]

1. It's not different it's just formalizing the create flow to be the one we actually support for SharedObjects. The current flow is always developer defines the factory using DDS.getFactory() in their registry. Developer using DDS.create() to create a new instance. Fluid connects the two using the DDS.Type in the create call.

2. I would disagree because this change is explicitly scoping the API to things the developer needs. This doesn't change anything the developer currently needs to do with factories. The flow says exactly the same. The only difference is that the constructor can't be called.

3. I don't think I follow this. There is a current dependency both ways. This doesn't change any dependencies it just makes it so the TaskManager object constructor is injectable as opposed to defined. If anything it's adding dependency injection.

4. The problem here is that because of the lifecycle of SharedObjects you can't have a constructor create. The constructor is required to both create new instances as well as loading existing ones. This is why all of our DDSes use a explicit create. Having a public constructor that you can't actually use to create a new object is confusing. I 100% agree that constructors >> static create but that's not how we are currently built.

[tylerbutler]

Having a public constructor that you can't actually use to create a new object is confusing.

Agreed.

I would disagree because this change is explicitly scoping the API to things the developer needs. This doesn't change anything the developer currently needs to do with factories. The flow says exactly the same. The only difference is that the constructor can't be called.

I think the factory feedback is real and relevant, but also, any decision about moving away from factories needs some design work before we can really consider it. While this change does seem to "double down" on the factory, it doesn't change the overall shape of the system. We don't want people using constructors today, so hiding them seems appropriate.

I think the higher-level discussion is relevant to take up elsewhere. That is, "how do we address the factory pattern feedback we've heard?"

[ChumpChief]

At the bare minimum I don't think we should do this piecemeal -- all of our other DDSs have public constructors and match the current pattern in TaskManager. Having discrepancies is IMO harder to explain and harder to maintain.

I see that Mark/Vlad have an issue tracking this topic #1845 - I would suggest deferring changes here to be aligned with that work.

Introduces a "real" dependency from TaskManager to TaskManagerFactory (current dependencies are just convenience static functions)

I don't think I follow this. There is a current dependency both ways. This doesn't change any dependencies it just makes it so the TaskManager object constructor is injectable as opposed to defined. If anything it's adding dependency injection.

Vlad kind of mentions this in the linked issue above, but create() is just a thin wrapper on IFluidDataStoreRuntime.createChannel() and getFactory() is just a convenient (but in my opinion inappropriate) place to stash the factory so someone else can retrieve it. This is why I say they're not real dependencies -- the instance of the class doesn't depend on the factory at all. As demonstrated in the current test, the factory can be cut out of the picture and still permit exercise of the instance. Whereas with this change there's no way to create the instance without the factory involved - the instance becomes coupled to the factory.

And to be clear, I'm not trying to defend the current approach as being fantastic. But in the spirit of "change is bad unless it's great" I feel this has too many negatives for me to feel good about it.

[tylerbutler]

All protected APIs are marked @internal to remove them from the API docs

Does JS have a final for classes? It seems like we don't think this class should be subclassed, and final would clarify that intent.

Update: To answer my own question: it appears not.

[skylerjokiel]

All protected APIs are marked @internal to remove them from the API docs

Does JS have a final for classes? It seems like we don't think this class should be subclassed, and final would clarify that intent.

Update: To answer my own question: it appears not.

There is no JS enforced final but you can use @Sealed in the docs which requires an explicit @OverRide if you do extend. This only works if that developer is also running the api-extractor though.

[skylerjokiel]

closing this since it was designed to see how people felt about a large amount of changes.