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

SharedObject.is() should be removed, possibly replaced with more generic check #8594

Closed
vladsud opened this issue Dec 20, 2021 · 2 comments · Fixed by #8841
Closed

SharedObject.is() should be removed, possibly replaced with more generic check #8594

vladsud opened this issue Dec 20, 2021 · 2 comments · Fixed by #8841
Assignees
Labels
bug Something isn't working
Milestone

Comments

@vladsud
Copy link
Contributor

vladsud commented Dec 20, 2021

Looking at the code, it is used to call SharedObject.bindToContext()
I see two problems with that:

  • there is nothing unique about SharedObject. I can build another base class for another set of DDSs and it should work exactly the same way.
  • I think the actual check and actions are wrong, because we moved to using handles and objects themselves should never be stored in DDSs. Some code actually correctly validates that:
    public set(value: Serializable<T>) {
        if (SharedObject.is(value)) {
            throw new Error("SharedObject sets are no longer supported. Instead set the SharedObject handle.");
        }

But most of the code seems to be wrong:

    protected registerCore() {
        for (const value of this.intervalMapKernel.values()) {
            if (SharedObject.is(value)) {
                value.bindToContext();
            }
        }
@vladsud vladsud added the bug Something isn't working label Dec 20, 2021
@vladsud vladsud added this to the January 2022 milestone Dec 20, 2021
@vladsud
Copy link
Contributor Author

vladsud commented Dec 20, 2021

@scarlettjlee, Is this something you can help cleaning in January?
@DLehenbauer - can you please glance at usage and confirm / deny my observations?

If we remove these checks completely, is there anything else we can do to help developers realize they are making mistake by putting some complex (non-serializable) object into DDS? I think @tylerbutler run into it or was passionate about it, so tagging you here too - the more the merrier :)

@DLehenbauer
Copy link
Contributor

DLehenbauer commented Jan 2, 2022

I agree that we can remove this code. It was an aid to help users transition when we introduced IFluidHandle.

The call to 'bindToContext()' was likely to allow old documents to load, but I think this is predating the 'Fluid Preview' app and ODSP.

In TypeScript, I'm pretty happy with the defense we get from the new improved `Serializable'.

If we want to add runtime checks for ES6 users, I'd suggest mixing these into the existing recursive descent we do during serialization to encode handles to minimize the runtime impact.

However, ES6 users live in an unsafe world, so these sorts of runtime checks have diminishing returns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants