Deprecate a few more things before we launch v2 #348
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important The terms of service for this installation has not been accepted. Please ask the Organization owners to visit the Gemini Code Assist Admin Console to sign it. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request deprecates some APIs in the package:web library, which is a good step towards API stabilization. The changes look good overall, and the deprecation messages are helpful. I've added a few suggestions for minor improvements.
Summary of Findings
- Deprecation messages: The deprecation messages should clearly indicate the preferred alternative and the reason for deprecation.
- Version update: The version update in
pubspec.yamlandCHANGELOG.mdshould be consistent.
Assessment
The pull request is well-structured and addresses the intended deprecations effectively. The code is clear and the deprecation messages are helpful. I recommend addressing the minor suggestions provided in the review comments before merging. Please ensure that others review and approve this code before merging.
Comment on lines
+3
to
+4
| - Deprecated `Node.text` extension. Use `Node.textContent` instead. | ||
| - Deprecated `[]` extensions on `Storage`. |
There was a problem hiding this comment.
It would be helpful to include the reason for deprecation in the changelog entry. This gives users more context as to why the change was made.
Comment on lines
+73
to
74
| @Deprecated('See Node.textContent') | ||
| set text(String s) => textContent = s; |
There was a problem hiding this comment.
Consider adding a more descriptive message to the deprecation annotation, explaining why Node.text is deprecated and what the user should do instead. This will help users understand the reasoning and migrate their code more easily.
@Deprecated('Use Node.textContent for setting text content. Node.text is deprecated for consistency with the DOM API.')
Comment on lines
+102
to
103
| @Deprecated('Use Storage.getItem instead') | ||
| String? operator [](String key) => getItem(key); |
There was a problem hiding this comment.
Similar to the previous comment, adding a more descriptive message to the deprecation annotation for StorageGlue would be beneficial. Explain why the operator is deprecated and what the recommended alternative is.
@Deprecated('Use Storage.getItem(key) instead of the [] operator. The operator is deprecated for clarity and consistency.')
Comment on lines
+104
to
105
| @Deprecated('Use Storage.setItem instead') | ||
| void operator []=(String key, String value) => setItem(key, value); |
There was a problem hiding this comment.
Same comment as above, adding a more descriptive message to the deprecation annotation for StorageGlue would be beneficial. Explain why the operator is deprecated and what the recommended alternative is.
@Deprecated('Use Storage.setItem(key, value) instead of the []= operator. The operator is deprecated for clarity and consistency.')
srujzs
reviewed
Feb 26, 2025
srujzs
approved these changes
Feb 26, 2025
kevmoo
commented
Feb 26, 2025
|
v2 of what?? |
|
|
copybara-service bot
pushed a commit
to dart-lang/sdk
that referenced
this pull request
Mar 7, 2025
…, tools, vector_math, web, webdriver Revisions updated by `dart tools/rev_sdk_deps.dart`. dartdoc (https://github.com/dart-lang/dartdoc/compare/34561d6..6c770bf): 6c770bfb 2025-03-01 dependabot[bot] Bump the github-actions group with 5 updates (dart-lang/dartdoc#4011) 06233c76 2025-02-26 Parker Lougheed Use nested scss in nav and footer styles (dart-lang/dartdoc#4006) 4847677c 2025-02-26 Sam Rawlins Support the stats flag when documenting the testing package (dart-lang/dartdoc#4008) ecosystem (https://github.com/dart-lang/ecosystem/compare/06bbbff..a3cc42d): a3cc42d 2025-03-06 Devon Carew Update publish_internal.yaml (dart-lang/ecosystem#345) 25f0fb2 2025-03-04 Devon Carew Update README.md (dart-lang/ecosystem#344) 331d35d 2025-03-01 dependabot[bot] Bump the github-actions group with 5 updates (dart-lang/ecosystem#343) i18n (https://github.com/dart-lang/i18n/compare/06a664f..bdeec25): bdeec25 2025-03-04 Moritz Publish `package:intl4x: 0.11.0` (dart-lang/i18n#953) d84a927 2025-03-04 Moritz Update publishing workflow 547ce9f 2025-03-01 dependabot[bot] Bump the github-actions group with 2 updates (dart-lang/i18n#952) c6b911b 2025-02-21 dependabot[bot] Bump the github-actions group across 1 directory with 4 updates (dart-lang/i18n#949) c3e9fe2 2025-02-21 Moritz Update to new native_assets (dart-lang/i18n#941) protobuf (https://github.com/dart-lang/protobuf/compare/610943a..7838e44): 7838e44 2025-03-07 Ömer Sinan Ağacan Prefix grpc method 'call' and 'request' arguments with '$' to avoid shadowing user methods with the same name (google/protobuf.dart#964) 125fe9c 2025-03-02 dependabot[bot] Bump dart-lang/setup-dart from 1.7.0 to 1.7.1 (google/protobuf.dart#965) fb77c7c 2025-03-02 dependabot[bot] Bump actions/cache from 4.2.0 to 4.2.2 (google/protobuf.dart#966) shelf (https://github.com/dart-lang/shelf/compare/b39e611..2af8529): 2af8529 2025-03-04 Devon Carew Update publish.yaml (dart-lang/shelf#473) f5ae797 2025-03-01 dependabot[bot] Bump the github-actions group with 2 updates (dart-lang/shelf#471) sync_http (https://github.com/dart-lang/sync_http/compare/47e6b26..dc54465): dc54465 2025-03-02 dependabot[bot] Bump dart-lang/setup-dart from 1.7.0 to 1.7.1 (google/sync_http.dart#54) test (https://github.com/dart-lang/test/compare/a833663..9e349d0): 9e349d0e 2025-03-06 Jonas Finnemann Jensen Suggest using `dart pub add dev:checks` (dart-lang/test#2467) e941dbac 2025-03-04 Devon Carew Update publish.yaml (dart-lang/test#2465) 32bf9b73 2025-03-01 dependabot[bot] Bump the github-actions group with 5 updates (dart-lang/test#2462) tools (https://github.com/dart-lang/tools/compare/b51f39d..d67cd00): d67cd002 2025-03-07 Parker Lougheed [pub_semver] Remove dependency on `package:meta` (dart-lang/tools#2021) 04667d7e 2025-03-06 Parker Lougheed [pub_semver] Discourage modification of properties intended to be unmodifiable (dart-lang/tools#2020) b23129b9 2025-03-04 Devon Carew Update publish.yaml (dart-lang/tools#2025) 9765c2aa 2025-02-25 Parker Lougheed [package_config] Implement relational operators for `LanguageVersion` (dart-lang/tools#2016) vector_math (https://github.com/google/vector_math.dart/compare/533c513..f08d7d2): f08d7d2 2025-03-01 dependabot[bot] Bump dart-lang/setup-dart in the github-actions group (google/vector_math.dart#340) web (https://github.com/dart-lang/web/compare/c2d5f63..4854093): 4854093 2025-03-04 Srujan Gaddam Add pull request and id-token write permissions to publish.yaml (dart-lang/web#351) 33ed133 2025-03-01 dependabot[bot] Bump the github-actions group with 2 updates (dart-lang/web#349) 6031c1f 2025-02-26 Kevin Moore Delete all deprecated members (dart-lang/web#347) 5a39fdc 2025-02-26 Kevin Moore Merge tag 'web-v1.1.1' 222164b 2025-02-26 Kevin Moore Deprecate a few more things before we launch v2 (dart-lang/web#348) b90b63d 2025-02-25 Kevin Moore Update APIs and docs (dart-lang/web#345) 5caa92e 2025-02-25 Srujan Gaddam Add catch and finally to bannedNames (dart-lang/web#346) e2f9741 2025-02-25 Olzhas Suleimen Add variadic arguments (dart-lang/web#343) webdriver (https://github.com/google/webdriver.dart/compare/b4fd859..f52afbf): f52afbf 2025-03-06 Parker Lougheed Utilize switch expressions (google/webdriver.dart#327) ee311e0 2025-03-02 dependabot[bot] Bump dart-lang/setup-dart from 1.7.0 to 1.7.1 (google/webdriver.dart#328) b0cb6a9 2025-02-27 Kevin Moore Update to Dart/Flutter team lints and fix (google/webdriver.dart#322) d080ebf 2025-02-27 Kevin Moore Update a number of APIs to return Uint8List (google/webdriver.dart#323) 0400c06 2025-02-27 Kevin Moore Re-enable Firefox on CI (google/webdriver.dart#324) 38a6646 2025-02-24 Parker Lougheed Minor cleanup for new and future lints (google/webdriver.dart#321) Change-Id: Ied703b048f85dd78e30084ffa8bffcaab5bab67b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/414301 Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Devon Carew <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.