Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
@uppy/aws-s3: add
endpoint
option #5173@uppy/aws-s3: add
endpoint
option #5173Changes from 5 commits
07f17e4
dfbd00d
971296c
9369e7e
5a19fc9
c9798bd
02beb51
35525ae
1686925
85278ec
f4dff7c
84deafc
6a61f67
e6f88c4
de54ce7
25680cd
a319f42
84883e4
f396841
21d1e1c
8b9dcd9
85dea61
88f47fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm okay with doing a breaking change. If not now then when?
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.
On the next major
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 still think this is a lot of code for backwards compatibility which we're not obliged to do. People can not blindly upgrade Uppy anyway, so you have to already check all things of the migration guide anyway. Why not just errors in the constructor, saying the properties are renamed?
Maybe we should let @mifi settle it, backwards compatible or breaking for 4.x.
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.
we are breaking many other apis in 4.x, so why not break this too?
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.
This does not have any backward compatiblity, it's just emitting warnings if folks are using the old option names.
Check failure on line 409 in packages/@uppy/aws-s3/src/index.ts
GitHub Actions / Type tests
Check failure on line 409 in packages/@uppy/aws-s3/src/index.ts
GitHub Actions / Type tests
Check failure on line 409 in packages/@uppy/aws-s3/src/index.ts
GitHub Actions / Browser tests
Check failure on line 409 in packages/@uppy/aws-s3/src/index.ts
GitHub Actions / Browser tests
Check failure on line 411 in packages/@uppy/aws-s3/src/index.ts
GitHub Actions / Type tests
Check failure on line 411 in packages/@uppy/aws-s3/src/index.ts
GitHub Actions / Type tests
Check failure on line 411 in packages/@uppy/aws-s3/src/index.ts
GitHub Actions / Browser tests
Check failure on line 411 in packages/@uppy/aws-s3/src/index.ts
GitHub Actions / Browser tests