Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Change HttpProfileRequestRef.id type from String to int #51035

Closed
CoderDake opened this issue Jan 17, 2023 · 8 comments
Closed

Change HttpProfileRequestRef.id type from String to int #51035

CoderDake opened this issue Jan 17, 2023 · 8 comments
Assignees
@CoderDake
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. breaking-change-request This tracks requests for feedback on breaking changes type-enhancement A request for a change that isn't a bug vm-service The VM Service Protocol, both the specification and its implementation

Comments

@CoderDake
Copy link
Contributor

CoderDake commented Jan 17, 2023 •
edited

Intent

  • Change HttpProfileRequestRef to use String for it's id type.
  • Change ext.dart.io.getHttpProfileRequest to use String for the passed id (instead of int).

Justification

flutter/DevTools runs on web, so it only has access to 52 bit int.
Using String to pass the id will avoid overflowing the id.

Impact

These RPC calls were originally created for use with Devtools. I haven't been able to find any other internal uses of the RPC.

I'm unsure of how to check if any external customers depend on these.

Mitigation

  • Anyone uses of the ext.dart.io.getHttpProfile or ext.dart.io.getHttpProfileRequest VmService calls
    • may need to change the types of ids that they pass or receive.
  • Uses of HttpProfileRequestRef or HttpProfileRequest
    • may need to change type of ids being passed or reveived

Changes

To see the specific proposed changes see: https://dart-review.googlesource.com/c/sdk/+/279122

⚠️ Update ⚠️ (Below was added Jan 19, 2023 @ 2:13PM ET)

While testing these changes I found that the SocketStatics.id type should be changed from int to String as well.
I am going to include that change in this breaking change as well.
@CoderDake CoderDake added the breaking-change-request This tracks requests for feedback on breaking changes label Jan 17, 2023
This was referenced Jan 17, 2023
Merged
Closed
@a-siva a-siva added vm-service The VM Service Protocol, both the specification and its implementation area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. labels Jan 18, 2023
@a-siva
Copy link
Contributor

a-siva commented Jan 18, 2023

//cc @bkonyi @derekxu16

@a-siva a-siva added the type-enhancement A request for a change that isn't a bug label Jan 18, 2023
@bkonyi bkonyi removed the area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. label Jan 18, 2023
@bkonyi
Copy link
Contributor

bkonyi commented Jan 18, 2023

This change SGTM.

@a-siva a-siva added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Jan 18, 2023
@bkonyi
Copy link
Contributor

bkonyi commented Jan 19, 2023

@itsjustkevin can you sign off on this please? :)

@CoderDake
Copy link
Contributor Author

CoderDake commented Jan 19, 2023 •
edited

⚠️ Update ⚠️
While testing these changes I found that the SocketStatics.id type should be changed from int to String as well.
I am going to include that change in this breaking change as well.

@CoderDake CoderDake self-assigned this Jan 19, 2023
@itsjustkevin
Copy link
Contributor

@grouma @Hixie @vsmenon could you take a look at this breaking change request?

@vsmenon
Copy link
Member

vsmenon commented Feb 3, 2023

lgtm

@grouma
Copy link
Member

grouma commented Feb 3, 2023

LGTM. This may have an impact on devtools, cc @kenzieschmoll

@CoderDake
Copy link
Contributor Author

The change has now been landed from https://dart-review.googlesource.com/c/sdk/+/280020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@CoderDake CoderDake

Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. breaking-change-request This tracks requests for feedback on breaking changes type-enhancement A request for a change that isn't a bug vm-service The VM Service Protocol, both the specification and its implementation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@CoderDake @grouma @vsmenon @a-siva @itsjustkevin @bkonyi