Skip to content

Conversation

@div5yesh
Copy link
Contributor

Issue #, if available: #5218

Description of changes:

  • encode custom state using base16 to preserve state from Cognito's URI encoding.
  • add base16 encoding/decoding util functions to aws-amplify/core.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #6851 into main will decrease coverage by 0.12%.
The diff coverage is 77.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6851      +/-   ##
==========================================
- Coverage   73.48%   73.35%   -0.13%     
==========================================
  Files         205      213       +8     
  Lines       12621    13153     +532     
  Branches     2460     2566     +106     
==========================================
+ Hits         9275     9649     +374     
- Misses       3157     3310     +153     
- Partials      189      194       +5     
Impacted Files Coverage Δ
packages/api-graphql/src/types/index.ts 100.00% <ø> (ø)
packages/datastore/__tests__/schema.ts 100.00% <ø> (ø)
packages/datastore/src/storage/storage.ts 71.66% <ø> (ø)
packages/datastore/src/sync/outbox.ts 24.00% <0.00%> (-0.49%) ⬇️
packages/datastore/src/sync/processors/mutation.ts 13.19% <0.00%> (ø)
...rc/Providers/AmazonAIConvertPredictionsProvider.ts 61.01% <ø> (ø)
...pubsub/src/Providers/AWSAppSyncRealTimeProvider.ts 18.37% <0.00%> (-0.17%) ⬇️
packages/pushnotification/src/PushNotification.ts 51.72% <ø> (ø)
packages/datastore/src/sync/index.ts 15.41% <9.09%> (-0.04%) ⬇️
packages/datastore/src/sync/processors/sync.ts 16.66% <18.18%> (-0.44%) ⬇️
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df83573...66cc378. Read the comment docs.

Copy link
Contributor

@ericclemmons ericclemmons left a comment

Choose a reason for hiding this comment

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

This is looking great @div5yesh! I'm ready to approve, but would want to see a couple tests validating this since we ran into interesting behavior.

unit tests for url safe encoding/decoding
Copy link
Contributor

@ericclemmons ericclemmons left a comment

Choose a reason for hiding this comment

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

This looks good!

The only concern I have is if this patch release will break existing IE 11 apps due to the use of TextEncoder.

IE 11 usage requires a good deal of polyfills by customers today, so needing a polyfill won't be new, but I'd like to also avoid breaking their apps the next time they npm install.

If we're already using TextEncoder elsewhere, I think we're good! Otherwise, are we converting strings to streams differently? (This implementing looks like what we needed to re-use toHex/fromHex)

@elorzafe What are your thoughts?

Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Thanks @div5yesh ! 👍 🌮 🎉

@div5yesh div5yesh merged commit 8cc5587 into aws-amplify:main Sep 25, 2020
nubpro pushed a commit to nubpro/amplify-js that referenced this pull request Oct 2, 2020
…lify#6851)

* bugfix aws-amplify#5218: de/encode custom state using base16

* use aws-sdk hex encoding util

unit tests for url safe encoding/decoding

* avoid using TextEncoder to support IE11

Co-authored-by: Sam Martinez <samlmar@amazon.com>
iartemiev pushed a commit that referenced this pull request Oct 13, 2020
* bugfix #5218: de/encode custom state using base16

* use aws-sdk hex encoding util

unit tests for url safe encoding/decoding

* avoid using TextEncoder to support IE11

Co-authored-by: Sam Martinez <samlmar@amazon.com>
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants