Add web url launcher #2119
Add web url launcher #2119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good.
One minor detail - but one that we should decide on one and will probably affect all platform implementations for all plugins: url_launcher_<platform>
or <platform>_url_launcher
? my tendency is to go with the former as that what we already used for fuchsia webview, and in the http://flutter.dev/go/federated-plugins which was reviewed by multiple people and no one commented on the naming...
I also feel like people the name should emphasize that this is a implementation more than that this is for the url_launcher plugin (especially once it get endorsed we don't expect people to find this plugin in pub and use it...)
Again it's a minor detail and I wouldn't spend the time discussing it normally, but once we publish there's no way back and as we'll probably want to be consistent across all plugins/platforms this is going to set the naming convention for all of them...
test('cannot launch "tel" URLs', () { | ||
expect(canLaunch('tel:5551234567'), completion(isFalse)); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to test launch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would require a lot of extra code. Rather than running the test using flutter test --platform chrome
we would have to spin up our own instance of Chrome, somehow build the JS getting the Flutter Web SDK from somewhere, call launch
, and determine that a new tab was opened with the correct API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some basic tests for launch
I think you mean the latter?
Except that the doc explicitly describes cases (API extension) where people will directly use the implementation package; in that case, having But in the directory structure we picked having the platform first is probably nicer in terms of navigating the repository, so I think both have relatively small pros and cons, and either is fine with me. |
The web implementation package doesn't include the dummy iOS and Android files, so adding this to a project that targets iOS and/or Android will break them. Do you want to add the workaround, or are we just waiting for the real fixes to the Flutter plugin tooling on that? |
Oops, yes the latter. |
I have a slight preference for |
FYI, I changed the |
Add url_launcher_web package Test plugins even if they don't have a pubspec in the top-level folder Bump up the pubspec version for new homepage URL Add dummy podspec file for iOS Remove unused import Format test Update check_publish.sh script
I updated the PR so the tests run, and the publish check works properly, PTAL |
Ok, lets keep the current name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments on the CI scripts, I think that change probably worth a separate PR though.
script/common.sh
Outdated
CHANGED_PACKAGES="${CHANGED_PACKAGES},$package" | ||
CHANGED_PACKAGE_LIST=("${CHANGED_PACKAGE_LIST[@]}" "$package") | ||
fi | ||
CHANGED_PACKAGES="${CHANGED_PACKAGES},$package" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the pubspec.yaml logic was introduced to prevent a deleted package to be running on CI, maybe we can just check that the directory exists instead?
cc @gspencergoog who originally introduce this logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to check if the directory exists
@@ -12,8 +12,8 @@ source "$SCRIPT_DIR/common.sh" | |||
|
|||
function check_publish() { | |||
local failures=() | |||
for package_name in "$@"; do | |||
local dir="$REPO_DIR/packages/$package_name" | |||
for dir in $(pub global run flutter_plugin_tools list --plugins="$1"); do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems wrong to only look at the first argument (prior to this change it would check the packages for all arguments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was because previously, check_publish
was called with the dereferenced CHANGED_PACKAGE_LIST
array, which passed the array items as separate arguments. Now there is only one argument, the packages list in a comma-separated list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Follows the structure established in flutter#2119 to add a federated macOS implementation of the url_launcher plugin. Fixes macOS portion of flutter/flutter#41721
* Move url_launcher to url_launcher/url_launcher Add url_launcher_web package Test plugins even if they don't have a pubspec in the top-level folder Bump up the pubspec version for new homepage URL Add dummy podspec file for iOS Remove unused import Format test Update check_publish.sh script * UPdate check_publish for federated plugins. * Follow correct semver when bumping version * Add some tests for `launch` * Check if directory exists for CI
* Move url_launcher to url_launcher/url_launcher Add url_launcher_web package Test plugins even if they don't have a pubspec in the top-level folder Bump up the pubspec version for new homepage URL Add dummy podspec file for iOS Remove unused import Format test Update check_publish.sh script * UPdate check_publish for federated plugins. * Follow correct semver when bumping version * Add some tests for `launch` * Check if directory exists for CI
Description
Adds the
url_launcher_web
plugin, the Web platform implementation ofurl_launcher
.Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?