Skip to content

Refactor AppDocument to rename name field to documentName. #4564

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vikdubey
Copy link

@vikdubey vikdubey commented Apr 21, 2025

Refactor AppDocument to rename name field to documentName.

flutter/flutter#167426

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment.

If you'd like to keep iterating on this I can give more pointers.

@@ -116,7 +116,15 @@ abstract class AppDocument<T extends AppDocument<T>> implements g.Document {
final Map<String, g.Value> _fields;

@override
String? name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this doesn't really solve the underlying problem, now there is a name and documentName, but you can still use say, task.name and accidentally be referring to the full database/path/to/document instead of task.taskName.

I appreciate the contribution but this isn't enough to warrant merging.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @matanlurey for the prompt review and feedback.

I understand the issue with making both name and documentName accessible. What about we make the name field unaccessible like below,

@OverRide
String? get name {
throw UnsupportedError(
'Accessing name is not allowed. Use documentName instead.');
}

@OverRide
set name(String? value) {
throw UnsupportedError(
'Setting name is not allowed. Use documentName instead.');
}

Please let me know if this would work, I can make code changes accordingly. If not, could you please provide some pointers on potential design. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately that won't be a safe change to make - <Document>.name is used by the underlying (generated) googeapis.v1.firebase package, so it needs to exist, we just don't want to use it (directly). There are a few things we could do, in order of easiest to most difficult:

  1. Mark String get/set name as @protected. Firebase APIs can still use, we won't (at least not by accident)
  2. Make AppDocument no longer implement/extend Document, and instead using composition

Given the work you already did on this PR, I'd be OK with going with (1) as a stop-gap (it makes the current situation better), and leaving a bug open for (2). What do you think?

@vikdubey vikdubey requested a review from matanlurey April 22, 2025 09:45
@vikdubey vikdubey marked this pull request as draft April 25, 2025 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants