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

storage(swift): update swift storage with storage path #7142

Closed
wants to merge 9 commits into from

Conversation

phantumcode
Copy link
Contributor

Description of changes:

  • Update Swift Storage category with documentation with new APIs that uses StoragePath and deprecate old APIs

Related GitHub issue #, if available:

Instructions

If this PR should not be merged upon approval for any reason, please submit as a DRAFT

Which product(s) are affected by this PR (if applicable)?

  • amplify-cli
  • amplify-ui
  • amplify-studio
  • amplify-hosting
  • amplify-libraries

Which platform(s) are affected by this PR (if applicable)?

  • JS
  • Swift
  • Android
  • Flutter
  • React Native

Please add the product(s)/platform(s) affected to the PR title

Checks

  • Does this PR conform to the styleguide?

  • Does this PR include filetypes other than markdown or images? Please add or update unit tests accordingly.

  • Are any files being deleted with this PR? If so, have the needed redirects been created?

  • Are all links in MDX files using the MDX link syntax rather than HTML link syntax?

    ref: MDX: [link](https://docs.amplify.aws/)
    HTML: <a href="https://docs.amplify.aws/">link</a>

When this PR is ready to merge, please check the box below

  • Ready to merge

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

@phantumcode phantumcode changed the title [DRAFT] - storage(swift): update swift storage with storage path [DRAFT] storage(swift): update swift storage with storage path Mar 29, 2024
@phantumcode phantumcode marked this pull request as ready for review April 9, 2024 16:45
@phantumcode phantumcode requested review from hdworld11 and a team as code owners April 9, 2024 16:45
@phantumcode phantumcode changed the title [DRAFT] storage(swift): update swift storage with storage path storage(swift): update swift storage with storage path Apr 9, 2024
@@ -4,6 +4,51 @@ There are three ways of getting data that was previously uploaded:

You can download to in-memory buffer [Data](https://developer.apple.com/documentation/foundation/data) object with `Amplify.Storage.downloadData`:

#### With StoragePath
Copy link
Member

Choose a reason for hiding this comment

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

We should add a short blurb about how this is new in the latest version and we recommend customers to use this.

Copy link
Contributor Author

@phantumcode phantumcode Apr 12, 2024

Choose a reason for hiding this comment

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

I've added a callout for each of the new API. I didn't specify a specific Amplify version, as this callout can be common and used across all native platforms.

Comment on lines 1 to 2
Delete an object uploaded to S3 by using `Amplify.Storage.remove` and specify the key:

Copy link
Member

Choose a reason for hiding this comment

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

should stop using key terminology here.

Copy link
Member

Choose a reason for hiding this comment

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

Suggestion to do:

Delete an object uploaded to S3 by using Amplify.Storage.remove. Below then shows the "with" options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated per suggestion

<Block name="Async/Await">

```swift
let removedKey = try await Amplify.Storage.remove(path: .fromString("example/path"))
Copy link
Member

Choose a reason for hiding this comment

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

should stop using key terminology here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

key terminology removed.

@@ -1,7 +1,59 @@
## Upload data

To upload to S3 from a data object, specify the `key` and the `data` object to be uploaded.
Copy link
Member

Choose a reason for hiding this comment

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

Remove "key" terminology here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

key terminology removed.


This will list all public files (This behavior is deprecated):
Copy link
Member

Choose a reason for hiding this comment

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

I would just remove this line and add #### With Key (Deprecated) like used elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated per suggestion.

@@ -1,7 +1,50 @@
You can list all of the objects uploaded under a given prefix by setting the `pageSize`. If the `pageSize` is set lower than the total file size available, A single `Storage.list` call only returns a subset of all the files. To list all the files with multiple calls, the user can use the `nextToken` from the previous call response.

This will list all public files:
This will list all files in the path:
Copy link
Member

Choose a reason for hiding this comment

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

I would just remove this and use #### With StoragePath as you have done elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated per suggestion

@@ -59,6 +111,88 @@ let resultSink = uploadTask

When you have a file that you want to upload, you can specify the url to the file in the `local` parameter.
If a file with the same `key` already exists in S3, the existing S3 file will be overwritten.
Copy link
Member

@tylerjroach tylerjroach Apr 11, 2024

Choose a reason for hiding this comment

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

key is problematic here.

Android may just use:

To upload data to S3 from a File:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced key with path

Copy link
Member

@tylerjroach tylerjroach left a comment

Choose a reason for hiding this comment

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

getting-started/common.mdx has: "To upload to S3 from a data object, specify the key and the data object to be uploaded." This is applied to all platforms and needs to be adjusted.

It also says: "It should contain a file called ExampleKey, whose contents is Example file contents." We should maybe adjust that file name to prevent confusion.

Copy link

Amplify Docs is moving away from the use of Fragments. Please instead use InlineFilter. See our README for more information.

Copy link

Amplify Docs is moving away from the use of Fragments. Please instead use InlineFilter. See our README for more information.


</BlockSwitcher>

#### With Key (Deprecated)
Copy link
Member

Choose a reason for hiding this comment

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

Let's add context that this has been deprecated from the latest version and we recommend using StoragePath above instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be a callout at the top of the download page now.

@@ -1,3 +1,4 @@
#### Note: This feature is deprecated
Copy link
Member

Choose a reason for hiding this comment

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

We should provide more context on why this is deprecated and what the customer should do instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a callout warning. This is the same/consistent with the JS Gen2 Storage doc updates.

Copy link

Amplify Docs is moving away from the use of Fragments. Please instead use InlineFilter. See our README for more information.

Copy link

Amplify Docs is moving away from the use of Fragments. Please instead use InlineFilter. See our README for more information.

harsh62
harsh62 previously approved these changes Apr 16, 2024
hdworld11
hdworld11 previously approved these changes Apr 22, 2024
@phantumcode phantumcode dismissed stale reviews from hdworld11 and harsh62 via 8abfd81 April 22, 2024 19:30
@phantumcode phantumcode deleted the feature/swift_gen2_storage branch April 22, 2024 19:46
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