flutterfire
flutterfire copied to clipboard
refactor(vertexai): Split into separate libraries
The current approach uses part files which cannot be imported
separately. One drawback of this approach is that any members which need
to be referenced by test code must be public to the library. Using
src/ libraries, as opposed to src/ part files, allows making APIs
that are "package private" by convention - they are public to the
library, but only exported through the src/ import.
One potential downside (alternatively seen as a benefit) of using separate libraries is that private members are truly private to their library, and we cannot make a member of a class private to the package. The convention for package imports only allows top level declarations to be considered non-public implementation details.
- Remove imports from the top level library. Replace
partlines withexportof the same files. - Add explicit
showclauses to the exports. This is what allows an API to be public when imported from thesrc/library, but not exposed through the package public library. - Change some private
_toGoogleAI<TypeName>methods to extension methods. Use the nametoGoogleAIsince the specific type names are clear from context. - Change some private
_fromGoogleAI<TypeName>factory constructors to extension methods on the google AI SDk types. NametoVertex(). - Remove some unused private methods that were filled in for consistency. More extension members can be added as needed.
- Add an extension to expose the private field
_googleAiModelfromGenerativeModel. - Add a top level method to forward to the private
GenerativeModelconstructor. - Add TODO comments to stop exporting some methods that don't need to be
public. These could have been private in the
partpattern. For now they are exported for backwards compatibility, and we can remove them when we are ready for a major version bump.
If you have any branches that don't merge cleanly with this I can hold off and land it whenever it's convenient.
This is a refactor I would recommend, but it's not urgent. Let me know if you have any questions about the reasoning or implications of this change.
If you want to go forward with this refactoring I can work on jumping through the CI hurdles.
If you have any branches that don't merge cleanly with this I can hold off and land it whenever it's convenient.
This is a refactor I would recommend, but it's not urgent. Let me know if you have any questions about the reasoning or implications of this change.
If you want to go forward with this refactoring I can work on jumping through the CI hurdles.
This is great, thanks for doing this! I'd prefer you to go ahead and get this in first. It's much cleaner.
I've added some API docs to these members for now. I'd recommend dropping the lint which requires this - it currently doesn't understand "package private" APIs (which these all are), and we have found it to be the case that some APIs don't have any useful information to add - and in those cases we prefer omitting the docs to adding redundant docs.
[cloud_firestore/permission-denied] The caller does not have permission to execute the specified operation.
Are these flakes?
It looks like there might also be some failures on master branch - is it OK to merge on red CI in this repo?
@natebosch - you can ignore the firestore e2e as they relate to using second database which doesn't have emulator support and thus uses a live firebase project, I have app check enforcement on the firestore API as I am testing something out. When this PR gets to review state, I'll remove enforcement 😄, just ping me here (I think enforcement will be removed sooner to be honest).
OK. I think this should be good to go.