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.

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

[web] Allow the usage of url strategies without conditional imports #77103

Merged
merged 6 commits into from
Nov 2, 2021

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Mar 2, 2021

Today, users have to do conditional imports in their apps in order to customize URL strategy. @johnpryan suggested that we make it possible to use the package without having to do conditional imports. This PR is an attempt to achieve that.

Thanks @ditman for the help!

Fixes #81910

@mdebbar mdebbar added f: routes Navigator, Router, and related APIs. platform-web Web applications specifically labels Mar 2, 2021
@mdebbar mdebbar requested review from johnpryan and ditman March 2, 2021 22:47
@google-cla google-cla bot added the cla: yes label Mar 2, 2021
Copy link
Contributor

@johnpryan johnpryan left a comment

Choose a reason for hiding this comment

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

LGTM - are there ways to avoid copying the abstract classes? (I haven't used conditional imports much)

///
/// By default, the [HashUrlStrategy] subclass is used if the app doesn't
/// specify one.
abstract class UrlStrategy {
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend to export the common classes from another file, and only duplicate the things that really need a different implementation.

@goderbauer
Copy link
Member

(PR triage): Are there still plans for this PR?

@mdebbar
Copy link
Contributor Author

mdebbar commented Apr 26, 2021

@goderbauer yes, I'm planning to come back to this PR, address the review and land it.

@Hixie
Copy link
Contributor

Hixie commented Oct 19, 2021

(PR triage): Are there still plans for this PR?

@mdebbar mdebbar marked this pull request as ready for review October 27, 2021 15:21
@ditman
Copy link
Member

ditman commented Nov 1, 2021

info • Unnecessary type check; the result is always 'true' • packages/flutter_web_plugins/lib/src/navigation/url_strategy.dart:75:12 • unnecessary_type_check

@mdebbar mdebbar requested a review from ditman November 1, 2021 16:48
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

This LGTM! (Maybe somebody from Flutter Rollers can assess how problematic is the internal change required?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: routes Navigator, Router, and related APIs. platform-web Web applications specifically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setUrlStrategy should default to PathUrlStrategy
6 participants