Skip to content
This repository has been archived by the owner on Sep 15, 2021. It is now read-only.

create observe_base and move stuff there #77

Closed
skybrian opened this issue Oct 23, 2015 · 12 comments
Closed

create observe_base and move stuff there #77

skybrian opened this issue Oct 23, 2015 · 12 comments

Comments

@skybrian
Copy link

I'd like to move parts of the observe package to a different package to avoid a circular dependency.

We plan to use parts of the observe package in protobuf, and we're also thinking about using protobuf in the analyzer. So this creates a cycle analyzer->protobuf->observe->analyzer. Open source tools can handle circular dependencies but Google's build system (similar to bazel) cannot.

Even if we could handle the cycle, making the protobuf library depend on analyzer seems like a bad move given how many things depend on protobufs and how many things the analyzer depends on.

The classes I'd like to move to observe_base are:
Observable
ObservableList
ChangeRecord (used by Observable)
ListChangeRecord (used by ObservableList)
ChangeNotifier (used by ObservableList)

They would be imported and exported by observe, so callers should be unaffected.

@jmesserly
Copy link
Contributor

this is a great idea! but I'd go the other way: kick out transformer related stuff, into an observe_transform package or something like that.

That would help with several other issues too (#75, #9). I'd love to get out of the business of depending on analyzer in this package.

@jmesserly
Copy link
Contributor

BTW, is "observe" the main thing causing protobuf to depend on analyzer? If so I'm happy to take a high priority interrupt to help fix the situation. 100% agree on not having low level libs (like observe, protobuf) depend on analyzer.

CC @stereotype441

@sigmundch
Copy link
Contributor

Funny - I was about to write exactly what John said. Kicking out transformers has the disadvantage of being a breaking change (people will need to change their pubspects to add observe_transform), but I think it's worth it.

@skybrian
Copy link
Author

protobuf doesn't currently depend on the analyzer or observe. We're adding a mixin that depends on observe in Google's private fork and ran into a different circular dependency, which made me think of this. Eventually, observe support in protobufs might become a first-class feature, assuming the observe package is here to stay.

@skybrian
Copy link
Author

In particular, analyzer depends on watcher, so if a watcher needs to use a protobuf, things break.

@jmesserly
Copy link
Contributor

Oh! I thought @stereotype441 said protobufs depended on analyzer, so he was not going to use protobufs in analyzer (for summaries) to avoid the circular dependency. Maybe I misheard/understood that comment.

@sigmundch
Copy link
Contributor

... assuming the observe package is here to stay.

I believe so, but I do think we might have a change in the APIs going forward as we might simplify things and make them cheaper and more Darty in the long term (the current APIs derived from object.Observe used by polymer, I expect it will become more general).

Would it be possible to make the dependency weaker? say by allowing users to configure on the side how protobuf depends on a package like observe? (I just recall that you looked into doing something like it, so I'm not sure if that's still the plan)

@jmesserly
Copy link
Contributor

+1 ... I think ES Object.observe is not happening, for what it's worth.

Lots of things Siggi and I would like to fix about these APIs, now that we're free from a lot of those old constraints. Should be possible to make the Observe interface way simpler now.

@skybrian
Copy link
Author

I implemented a way to add mixins to generated protobuf classes. In theory, this avoids adding a dependency on package:observe to the protobuf package itself. In practice, Google's build system doesn't allow Dart dependencies to be customized for each proto library, and it may be a while until it does. So, if we want anyone to be able to use a mixin that uses package:observe, the most practical way is to put a dependency on package:protobuf.

I might not add the dependency in the open source package:protobuf yet (holding out for a better solution), but circular dependencies will still be an issue for Google.

Extracting out a separate observe_transform package would work for me.

Regarding the analyzer's usage of protobufs, for a while we were thinking "no", but we're back to "maybe" since it has faster performance now.

@jmesserly
Copy link
Contributor

Regarding the analyzer's usage of protobufs, for a while we were thinking "no", but we're back to "maybe" since it has faster performance now.

awesome! yeah, it seems good to reuse protos if possible.

@skybrian
Copy link
Author

Opened a separate issue for the new plan.

@stereotype441
Copy link
Contributor

Sorry for the confusion. I was referring to the circular dependency in
Google's private fork that Brian Slesinsky mentioned.

On Fri, 23 Oct 2015 at 13:07 John Messerly [email protected] wrote:

Oh! I thought @stereotype441 https://github.com/stereotype441 said
protobufs depended on analyzer, so he was not going to use protobufs in
analyzer (for summaries) to avoid the circular dependency. Maybe I
misheard/understood that comment.


Reply to this email directly or view it on GitHub
#77 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

4 participants