aws sdk v3

[jagthedrummer]

Fixes #1897

@kpfefferle If this looks good to you please let me know if there's anything I can do to help make it easier for you to release a new version.

[kpfefferle]

Released as v6.0.0

[jagthedrummer]

Awesome! 🎉

Thanks, @kpfefferle.

[ChrisN-transnexus]

@kpfefferle @jagthedrummer

The problem with this PR (which I realize is already merged and deployed) is that it uses the old, "v2 compatible" style for aws-sdk in the new v3 release. Amazon included this old syntax for ease of upgrading, but the v3 documentation advises against using it because it creates a larger bundle size and will most likely be removed (not just deprecated) in v4.

Considering that aws-sdk v2 had a ten year lifecycle, and v3 was released in 2020, this PR gives the end user/developer only about five years of runway. That means sometime in 2030, developers who rely on this plugin for enterprise software, like myself (and probably jagthedrummer), will be in the same situation we were a week ago.

A major goal of aws-sdk v3 was to reduce bundle size, but using the old v2 compatible style defeats that purpose. A test of DynamoDB found a 30% increase in bundle size by using the old v2 compatible style over the new v3 client syntax.

For these reasons, I made a PR which fully upgrades ember-cli-deploy-cloudfront to the v3 client syntax. I agree that my PR was larger, but it was also more future-proof, most likely giving this plugin at least 15 years of lifespan before more changes will be required. As this codebase now stands, in five years, someone is going to have to make the same changes I already proposed in my (now closed) PR.

I understand you are not actively using this plugin (and don't want to dedicate too much of your time to its maintenance), but clearly many other developers are, and it's not always the best course of action to merge the smallest, simplest PR when the lifespan of the code is potentially shortened by a decade.

At the very least, and in the spirit of open-source software, it would have been best to have an open discussion about the advantages and drawbacks of both PRs before unilaterally merging one and closing the other. If given the chance, I would have happily responded to any feedback regarding the scope of the changes I proposed and worked with anyone interested in fully upgrading ember-cli-deploy-cloudfront to aws-sdk v3 client syntax.

[jagthedrummer]

@ChrisN-transnexus, I can't speak for @kpfefferle, but I'd be interested in seeing a PR that only updates to the v3 syntax without doing anything else.

Speaking only for myself, as an open source maintainer I would not have merged your initial PR because it does too many things. And many of those things are more-or-less matters of personal preference, which have nothing to do with the aws-sdk v3 issue. Some of those things may even been worth doing, but it's much easier on reviewers and maintainers if things aren't jumbled together into a kind of "fix the whole project at once" monster PR.

For instance, changing var to const is probably a good idea, but doing that at the same time as actual functional changes make it much harder to evaluate either set of changes. var to const would be a good candidate for a stand alone PR which I personally would almost immediately give a ✅ and a merge (again speaking only for myself, and not as a maintainer of this project, so I don't actually have the ability to merge it). Changing function to () => {} syntax would be a good stand alone PR. Same with renaming variables. Removing the prettier stuff is definitely a matter of personal preference and it's presumptuous to just include that in an unrelated PR without any discussion.

If you want to take another shot at it I'd be happy to review and pull down your PR branch and give it a test run.

[ChrisN-transnexus]

@jagthedrummer Without a buy-in from the maintainer of this repo, I'm not going to invest more time scaling back my PR. I would have done so if I was asked to in my PR, before it was closed. That's the purpose of a "pull request", someone requests updates to the codebase and if there are concerns, they can be discussed and addressed.

Frankly, I would have liked the opportunity to comment on this PR, but it was merged less than 48 hours after it was posted. I didn't get an email for it because I wasn't tagged on it, and as someone with a similar PR opened 4 days earlier, I would have appreciated a heads-up.

The best place to address your concerns over my PR was in that PR, but I'll respond here.

Aside from fully updating the code to use aws-sdk v3's client/command syntax, I brought the repo up to ES6 standards, which have been available since 2015. I didn't think it was a huge issue to update two files plus a test file to standards that are now a decade old. There is no reason in modern JavaScript to ever use var self = this; To me, there is no need to explain or defend that kind of change. Same thing with .bind(this) - it's old code that should always be updated whenever it's found. I also changed some duplicated variable names to make them less confusing, and changed the order of the plugin.readConfig() code to not make unnecessary readConfig() calls if the user passes-in a custom client.

While it is usually preferable to make smaller PRs, I was being cognizant of the fact that kpfefferle has basically abandoned this repo and isn't interested in spending lots of his time maintaining it. If I were in that position, I would prefer one PR that brings the code fully up-to-date and future-proofs it for at least a decade over a slow drip-drip-drip of small PRs addressing one issue each. If kpfefferle did want multiple smaller PRs fixing all the out-of-date code, I would have made them.

I removed prettier because it is too opinionated and not at all configurable, and it makes simple pieces of code less human-readable. One of the worst things prettier does is breaking string concatenations into multiple lines, which makes them difficult to read. prettier is designed to enforce style between members of a team, and this plugin is being maintained by one or two people at this point, not a team. Regardless, I would have happily put prettier back in, if I was asked to. And it's not presumptuous to remove a non-code dependency in a PR "without any discussion." A PR is just a request, not a command. If the owner of the repo (or better, the community) had concerns, then that's when the discussion should take place.

I disagree that many of my changes were "matters of personal preference." Updating code to standards that have been around for a decade is not a personal preference, it's a best practice. Removing prettier was the one personal preference I included, but I would have put it back if asked.

While we're discussing these PRs, why did you choose to use the old v2 compatible style for aws-sdk v3 when the documentation recommends against that?

[jagthedrummer]

@ChrisN-transnexus from an "if it ain't broke, don't fix it" perspective many of your changes were indeed matters of preference. The old ways still work, even if there are different ways to do the same things that are currently in vogue.

And like I said before, many of your preferential change are worth doing, but having them mixed in amongst other changes makes it harder to review either set of changes. The preference for single-issue, one-conceptual-change per PR is a pattern I've seen repeated across literally dozens of projects, both open source and closed. It's just something to keep in mind if you want to increase your chances of having PRs accepted.

why did you choose to use the old v2 compatible style for aws-sdk v3 when the documentation recommends against that?

1. My PR was basically a cleanup of what the official migration tool (aws-sdk-js-codemod) suggested.
2. I was going for the simplest smallest change I could possibly make in order to make incremental progress on the immediate problem because I wanted to make it as easy as possible to review and merge.