-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix/geo location services config #13303
Fix/geo location services config #13303
Conversation
…/nadetastic/amplify-js into fix/geo-location-services-config
…ocation-services-config
…-location-services-config
cfef59c
to
27fb817
Compare
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.
Thanks for taking care of this! Do think the updates should be fine, but with the relevant code of this PR mirroring #12921, the concerns I had there have not changed:
-
This code has been in production for almost 6 months, what assurances do we have that this change won't cause any impact on existing usage? I'm sure that we've tested for regressions already, but can you please explain what exactly has been done testing and what safeguards are in place to ensure a lack of regression?
-
Regarding the actual root cause, need to see it addressed along with unit tests. A small targeted unit test of the Geo configuration would have prevented the issue initially, and can go a long way in catching potential regressions in the future
WoW....Really Appreciate doing it.. |
@@ -335,7 +335,7 @@ | |||
"name": "[API] generateClient (AppSync)", | |||
"path": "./dist/esm/api/index.mjs", | |||
"import": "{ generateClient }", | |||
"limit": "39.99 kB" | |||
"limit": "40.09 kB" |
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.
Perfectoooo
@@ -151,7 +151,7 @@ export class AmazonLocationServiceProvider implements GeoProvider { | |||
*/ | |||
let locationServiceInput: SearchPlaceIndexForTextCommandInput = { | |||
Text: text, | |||
IndexName: this._config.search_indices.default, | |||
IndexName: this._config.searchIndices.default, |
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.
GG : )
82690c8
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.
GG : )
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.
👍Always a nice change to eliminate every code path that could go wrong. Approve from JS side. I defer to @calebpollman to confirm potential UI change.
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.
🚢
Description of changes
Original PR: fix: (Geo) Kebab case to Camel case config props
Description of how you validated changes
Tested changes on sample app
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.