Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@chunhtai
Copy link
Contributor

@chunhtai chunhtai commented Aug 6, 2021

Fixes flutter/flutter#87823

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 and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@chunhtai chunhtai requested a review from yjbanov August 6, 2021 18:40
@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Aug 6, 2021
@google-cla google-cla bot added the cla: yes label Aug 6, 2021
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM

browserHistory.setRouteName(
arguments!.tryString('location'),
state: arguments.tryString('state'),
state: arguments['state'],
Copy link
Contributor

Choose a reason for hiding this comment

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

@yjbanov should we add a tryObject()?

Copy link
Contributor Author

@chunhtai chunhtai Aug 6, 2021

Choose a reason for hiding this comment

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

what is the point of tryObject though? it is already an object?

Copy link
Contributor

Choose a reason for hiding this comment

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

Without a type, it's dynamic not Object?. Not sure if that's an important distinction or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, feel free to submit the PR. If we need to change it to Object? we can do so later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add that if that makes sense, btw what is the motive to avoid as String? and use tryString

@chunhtai chunhtai merged commit ea4b353 into flutter:master Aug 6, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 9, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 11, 2021
filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022
* Fix routerinformationupdate can take complex json

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

Labels

cla: yes platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Router crashed if RouteInformation.state is not a string

2 participants