Rework README, prepare for packaging #2
Comments
|
Need to simplify the project itself. As I see it right now it consists of two separate, but linked, pieces - the backend that handles the security side - generating S3 authentication data and secure download links; and a Redux app that hooks into the Django endpoint and handles the file selection and upload. Django:
Redux JS:
|
|
@thealscott @ross - can you both confirm / deny what I've said above - am I understanding this project? |
|
@hugorodgerbrown that seems about right to me. Are you suggesting splitting them into two separate projects? |
|
Possibly - depends on how tightly coupled they are. Does the bare field in the admin site require the redux app? |
|
Looking further in to the matrix, I think there are two projects here. The first project is the Django endpoints required to support uploads and downloads to / from secure S3 buckets. This should have the minimal, unstyled, JS required to make it function. This project should be much smaller that it is currently - just the bare bones. The Redux piece is really a separate project that happens to hook into the backend via the HTML (the CSS selectors have to match) - however, since the template can be overridden this is a very loose coupling. The Redux project should include the JS, CSS and a new template. Practically speaking the redux piece could be inside the core platform project - it's quite specific to us. |
|
@hugorodgerbrown I don't think it is specific to the Platform, the Redux basically replicates the functionality of the old procedural script from before we branched, just using Redux paradigm to shift it to being declarative instead of imperative, and including some event dispatchers which allow external interfacing. I would say most of it IS the bare minimum. You could strip out some of the rendering logic that deals with hiding/showing/progressbar etc, but the bulk of the Redux is actually dealing with the AJAX bits |
|
@hugorodgerbrown I would say you have it about right in your summary (although I'm not sure which bit you're describing with "Custom TextInput widget"). I would also add that the JS provides events for external systems to respond to. That's how platform integrates with and customises the upload functionality (in "feature/6625-update-s3direct-usage-to-reflect-changes-in-yj-s3direct-project"). Personally, I think this is one project - what @thealscott has done is take the very basic JS from the original, bring it up to date and allow external projects to interface with its event system. The bloat is mostly in the example project, which certainly could be made less clunky if we're willing to spend the time. Also, if we're dropping support for older versions of Django and py2, that will simplify things significantly on the Python side of things. |
|
Would the upload-to-s3 function work in isolation without the redux project
(replaced with bare minimum JS)? If so, then it's two projects.
…On 20 July 2017 at 14:25, Ross Miller ***@***.***> wrote:
@hugorodgerbrown <https://github.com/hugorodgerbrown> I would say you
have it about right in your summary (although I'm not sure which bit you're
describing with "Custom TextInput widget"). I would also add that the JS
provides events for external systems to respond to. That's how platform
integrates with and customises the upload functionality (in
"feature/6625-update-s3direct-usage-to-reflect-changes-in-
yj-s3direct-project").
Personally, I think this is one project - what @thealscott
<https://github.com/thealscott> has done is take the very basic JS from
the original, bring it up to date and allow external projects to interface
with its event system.
The bloat is mostly in the example project, which certainly could be made
less clunky if we're willing to spend the time. Also, if we're dropping
support for older versions of Django and py2, that will also simplify
things significantly on the Python side of things.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMQ8IBJLKBpETlKGGjB_WYf8eF7nZrGks5sP1VYgaJpZM4OdHdL>
.
|
|
what do you consider "bare minimum" JS? My interpretation is that the minimal functionality the JS needs to provide is:
90% of the current JS code is all about achieving those core features. There's maybe 100 lines of code above and beyond that which deals with emitting events for external interface and displaying the progress bar, but that's about it. You could definitely get rid of the progress bar stuff, but beyond that you could only get more minimal with the JS if we don't do any DOM manipulation, and switch to a totally event driven model which offloads all interface responsibility (including displaying errors and success states) to the end user. |
|
Devil's advocate rephrase of HRB's comment: Could the upload-to-s3 function work as well with a (less featureful) vanilla-JS or jQuery implementation of what the redux component does? If so, and if this is a flag-waving project, it'd be nice if we could support the idea of various JS clients for it (ie, someone might contrib/create the jQ version) |
|
it could, in which case we would basically need to revert everything in this branch back to the original procedural script and tickle it a little so that it parses the security parameters and dispatches events. You would shave off a few kb from the filesize, (most of the added filesize from the refactor is to do with using "import" module syntax that gets transpiled; Redux itself is actually pretty tiny, minified it's ~5kb, gzipped it's ~2kb) but beyond that, I'm not really sure what the benefit to doing it that way is. Are we concerned that people won't understand it because it's using Redux pattern? If we want to have contributed JS clients then it doesn't make sense to have any JS included in the core package at all, it should just be an API, and then we can provide a client, and users can roll their own. I'm not sure how that affects the plug-and-playability of the project though. It's a bit of a deviation from the original S3Direct project use-case which was pretty fire and forget, with a little facility for customisation if you want to dig around in the guts. |
|
@hugorodgerbrown and @stevejalim the redux version is, as @thealscott says, pretty minimal in terms of its implementation. The redux pattern is now very popular and well understood within the JS community (and for very good reason - it's fantastic). The JS devs within YJ can work with this style of implementation much more easily than a non explicitly state managed implementation. This also goes for most JS devs I know in the wider community. It would feel to me, and I think I am safe in saying to @thealscott as well, like a big step backwards to remove it or change it to a JQuery or vanilla JS based implementation. |
|
@hugorodgerbrown and @stevejalim also, in my view, the current implementation would aid us in our goal of demonstrating that we take security seriously - the use of modern JS practices, with separation of concern and testable distinct units, makes it look like we take the testing and reliability of the front end seriously. I realise that the JS being in one file (as per the s3direct JS) looks superficially simpler. However, in terms of updates, bug fixing and general maintenance, what @thealscott has done is much simpler to debug and maintain. |
|
Just to clarify - I like the redux implementation, and 👏 all of it. My comments are about how we package this up for wider distribution. The critical aspects of the project are the generation of the S3 signed-request info and the secure download URL. For reference - the Heroku demo for this contains 75 lines of python and 50 lines of JS (approx). Again - nothing either of you have done will not be used in production - this is about its presentation to external users. |
As discussed, I'd like this to stand out as a project for us - it's important that we can demonstrate that we take security seriously, and being able to point people at this is part of that. It needs to be in an 'auditable' state. This means a bunch of fixes:
The text was updated successfully, but these errors were encountered: