Skip to content

Conversation

@haverchuck
Copy link
Contributor

Description of changes:
Creates _android and _ios federated plugins for cognito package.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@haverchuck haverchuck changed the base branch from main to federate/cognito February 10, 2022 22:11
@haverchuck haverchuck changed the base branch from federate/cognito to main February 10, 2022 22:11
@haverchuck haverchuck force-pushed the federate/cognito branch 3 times, most recently from b852b06 to 0a83fdc Compare February 28, 2022 23:11
@haverchuck haverchuck marked this pull request as ready for review March 1, 2022 17:31
@haverchuck haverchuck requested a review from a team as a code owner March 1, 2022 17:31
Copy link
Member

@Jordan-Nelson Jordan-Nelson left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Mostly nit comments. I think there are some files in the _android package that don't need to be there, and we should consider updating the version numbers in the change logs for the new packages

sdk: flutter
amplify_auth_cognito_android:
# When depending on this package from a real application you should use:
# amplify_auth_cognito_ios: ^x.y.z
Copy link
Member

Choose a reason for hiding this comment

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

    #   amplify_auth_cognito_ios: ^x.y.z

Should that be amplify_auth_cognito_android? Or maybe we just remove this if this assuming it is auto generated. Not sure we need need this in the platform packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@dnys1
Copy link
Contributor

dnys1 commented Mar 1, 2022

Can we follow the Flutter team on layout?

plugin_name/
    plugin_name/
    plugin_name_platform_interface/
    plugin_name_ios/
    plugin_name_android/

So, maybe something like this:

auth/
    amplify_auth_cognito/
    amplify_auth_cognito_ios/
    amplify_auth_cognito_android/
    amplify_auth_plugin_interface/

@@ -0,0 +1,3 @@
# amplify_auth_cognito_android_example
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the platform packages need examples still if all the tests are in the main pkg's example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've put the platform-specific unit tests in the platform specific packages.

use_frameworks!
use_modular_headers!

pod 'amplify_core', :path => './../../../../amplify_core/ios'
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be needed and if it is, then we need to figure out why

Copy link
Contributor Author

@haverchuck haverchuck Mar 2, 2022

Choose a reason for hiding this comment

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

[!] Unable to find a specification for `amplify_core` depended upon by `amplify_auth_cognito_ios`

Not totally sure why this happens (I would think the flutter_install_all_ios_pods command could handle this).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's because amplify_auth_cognito_ios is missing a dependency on amplify_core. You'll also need to bump the min iOS version to 13.0 because melos adds a dependency on DataStore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dnys1 Made the change!

@haverchuck haverchuck changed the base branch from main to amplify-authenticator March 1, 2022 23:22
@haverchuck haverchuck changed the base branch from amplify-authenticator to main March 1, 2022 23:23
@haverchuck haverchuck requested a review from dnys1 March 2, 2022 16:51
@haverchuck haverchuck merged commit a5399e4 into aws-amplify:main Mar 2, 2022
HuiSF added a commit that referenced this pull request Mar 11, 2022
* chore: Update issue template to GH form (#1387)

* chore: Update issue template to GH form

* Add N/A for placeholders

* Minor updates

* Minor fixes

* fix(amplify_authenticator): remove use of onInverseSurface to support older flutter versions (#1385)

* remove use of onInverseSurface

* set min flutter version to 2.0.0

* update fallback color

* chore(amplify_authenticator): run unit tests for amplify_authenticator in CI (#1389)

* fix(core): Implement Result in AtomicResult (#1393)

* fix(core): Implement Result in AtomicResult

* Fix AtomicResultTest

* chore: Small QOS additions to bug report template (#1394)

* fix(amplify_authenticator): allow multiple custom attributes in custom forms (#1377)

* add initial category for ui customization

* remove AuthState from the public API

* remove auth bloc from public api

* clean up public api, add docs

* update for consistency, fix typos

* change category name, fix typo

* update install docs

* update package description

* update example project dir

* uncomment example app imports

* update template names

* empty commit to trigger CI

* update how runtime and custom fields are handled

* move runtime fields to SignUpForm

* fix signout button

* Apply suggestions from code review

Co-authored-by: Dillon Nys <24740863+dnys1@users.noreply.github.com>

* rename includeDefaultFields, update sort

* Update packages/amplify_authenticator/lib/src/widgets/form.dart

Co-authored-by: Dillon Nys <24740863+dnys1@users.noreply.github.com>

* make _includeDefaultFields private

Co-authored-by: Dillon Nys <24740863+dnys1@users.noreply.github.com>

* chore: authenticator v0.1.0 (#1402)

* chore: authenticator v0.1.0

* Update packages/amplify_authenticator/pubspec.yaml

Co-authored-by: Dillon Nys <24740863+dnys1@users.noreply.github.com>

Co-authored-by: Dillon Nys <24740863+dnys1@users.noreply.github.com>

* chore: adds API for auth integ tests + minor fixes (#1302)

* chore(amplify_authenticator): add more detail to api docs for username (#1403)

* add more detail to api docs for username

* move docs to SignUpFormField

* fix overflow in custom loyout example (#1401)

* chore: rename docs dir to doc (#1406)

* chore(amplify_api): clean up some code to limit analysis infos (#1404)

* fix(datastore): delete default predicate causes deletion failure (#1409)

* fix(datastore): delete default predicate causes deletion failure

* fix save integration test

* Release 4.0.1 version bump (#1418)

* Version bump

* fix changelog 0.4.0 entry

* Add PR #s

* chore: add pub analysis to CI (#1408)

* add pana to ci

* add missing colon

* add install_melos step

* move scope arg

* move exec command to config

* remove pana from melos

* add remaining packages

* address issue in authenticator package

* disable failing packages

* revert change to melos.yaml

* add threshold

* add quotes

* escape quotes

* escape << >>

* escape <<<

* update variable parsing

* update parsing

* remove trailing quote

* add remaining packages

* update variable name, add desc

* adjust thresholds

* decrease api threshold

* chore: remove obsolete integration test driver files (#1395)

* fix(amplify_api): model helpers query predicates correctly translates… (#1417)

* chore(amplify_auth_cognito) federated plugin (#1349)

* chore(auth): fix pubspec urls (#1424)

Co-authored-by: Dillon Nys <24740863+dnys1@users.noreply.github.com>
Co-authored-by: Jordan Nelson <nejrd@amazon.com>
Co-authored-by: Travis Sheppard <tshepp@amazon.com>
Co-authored-by: Jordan Nelson <Jordan.Ryan.Nelson@gmail.com>
Co-authored-by: Dustin Noyes <dustin.noyes.dev@gmail.com>
Co-authored-by: Kyle <kylcheng@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants