feat(actions): standardize contexts
Following discussion on https://github.com/nextcloud-libraries/nextcloud-files/pull/1113
This would be a breaking change unless we add a conversion layer to support the old legacy way. BUT, the cleanest way, imho, is to quickly update server and communicate to devs asap
// Before
action.exec(node, view, '/path/to/node')
// After
action.exec({
nodes: [node],
view: View,
context: Folder
})
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 90.74%. Comparing base (fd3a895) to head (d9c2e53).
Additional details and impacted files
@@ Coverage Diff @@
## main #1124 +/- ##
=======================================
Coverage 90.74% 90.74%
=======================================
Files 23 23
Lines 670 670
Branches 182 182
=======================================
Hits 608 608
Misses 51 51
Partials 11 11
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
BUT, the cleanest way, imho, is to quickly update server and communicate to devs asap
I would try to make the library to work with all supported Nextcloud versions. This makes it hard to impossible to maintain multi-version apps (e.g. things like groupware). So I think we should make sure this works with 30, 29, and 28.
What I mean is we can do it like in this PR, which would be breaking and a v4 of the library. But it would then only be a breaking change of this library we should not break the server API which we should support as of our deprecation rules (at least 3 version, I do not want to go the 9 versions 3 years way :see_no_evil: )
So how to do it while not breaking server API?
Change the registerFileAction function in this PR to use a new _nc_fileactions_v2 to store them.
And in getFileActions we return a proxy over the real v1 actions emulating a v2 action (to simplify the server code).
This way an app using files v3 work with 28,29,30,31 as per our deprecation rules (we had this discussion with management at the Nextcloud conference). Apps using v4 will only work with 31+ but that is ok as long as v3 also works with 31,32 and 33. With 34 we can drop v3 support.
Context: There is no reason to not support a server version, features can be conditionally using capabilities. Binding a library to a server version removes all benefits from using a library. I think we can do breaking changes like this and release a new major, but should make sure it still works with the other Nextcloud versions by having so
Using 1 or 2 parameters before a context object in a function declaration does not seem that bad to me IMHO, any reason why we should never do this @skjnldsv?
Because it just looks weird to me. It's like mixing two syntaxes that have nothing in common, like spaces and tabs :grin:
From a clean declaration perspective, I would agree with
function(path, options = {})
function(path, folder, view...)
function(options = {})
But mixing something like function(path, view, dir, options = {}) seems incoherent to me.
To prove my point, and assuming there is no issue with mixing both, then why do this feels worse? function(options = {}, path, dir) or even worse: function(path, options = {}, view, dir) :see_no_evil:
But more importantly, I like to be consistent across our APIs here, and considering we have multiple APIs that do similar approaches, like FileAction, NewFileMenu, Views..., I think we should stick to one type of pattern here. That way it's easy to know what to expect when you work with the @nextcloud/files library.
This is why I was really against https://github.com/nextcloud-libraries/nextcloud-files/pull/1113 PS: https://github.com/nextcloud-libraries/nextcloud-files/pull/1135
Done @susnux ! Updated :rocket: