search

dart-archive / observe

Support for marking objects as observable, and getting notifications when those objects are mutated
https://pub.dartlang.org/packages/observe
BSD 3-Clause "New" or "Revised" License
13 stars 6 forks source link

create observe_base and move stuff there #77

Closed skybrian closed 9 years ago

skybrian commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

jmesserly commented 9 years ago

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 commented 9 years ago

... 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 commented 9 years ago

+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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

Opened a separate issue for the new plan.

stereotype441 commented 9 years ago

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 notifications@github.com 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 https://github.com/dart-lang/observe/issues/77#issuecomment-150677073.