[firebase_performance] Dart side of firebase_performance reformat

[bparrishMines]

Description

Dart side of possible reformat of Firebase Performance plugin

Related Issues

Android: #1529
iOS: #1556

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
• No, this is not a breaking change.

[collinjackson]

The general approach looks good to me, it needs updated tests I think

[collinjackson]

Previously if you called newTrace and never called start, it wouldn't leave anything hanging around. With this new approach the trace will leak. I'm not sure this is a major problem, but just wanted to point it out as a change.

[collinjackson]

Reminder to remove this if you don't intend to land it.

[collinjackson]

I guess this is just a matter of personal preference, are you sure you want to identify the singleton with a handle? There's just one on both the Dart and native side, and (AFAIK) _handle will always be zero. I guess an argument could be made for consistency with all the MethodChannel-using objects having a handle. It would be more obviously correct if FirebasePerformance had multiple app support.

I don't have a strong opinion here, just wanted to make sure you've thought about it.

[bparrishMines]

I do remember thinking about this. My goal was to try and separate the responsibility of the Plugin (FirebasePerformancePlugin) class from the main Firebase class (FirebasePerformance). My thinking would be that the plugin class would only be responsible for calling constructors and maintaining the MethodCallHandlers.

Also, i figured this wouldn't be as weird if it wasn't a Firebase plugin. Most libraries don't have a central class that most of their objects are created from.

And this would also set us up to easily integrate multi-app support if the firebase team decides to add it!

[bparrishMines]

The main reason of this restructure was that everything was called in start() and stop(), which prevented integration testing. Although this does open the classes up for leaking, we can now guarantee validity and avoid bugs such as this.

However, it would be possible to have both by only setting up the platform object when start is called and maintaining a list of () => invokeMethod()s until start is called.

[bparrishMines]

Closing in favor of #1557