nextcloud-files icon indicating copy to clipboard operation
nextcloud-files copied to clipboard

feat(actions): standardize contexts

Open skjnldsv opened this issue 1 year ago • 4 comments

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
})

skjnldsv avatar Nov 20 '24 08:11 skjnldsv

Bundle Report

Bundle size has no change :white_check_mark:

codecov[bot] avatar Nov 20 '24 08:11 codecov[bot]

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.

codecov[bot] avatar Nov 20 '24 08:11 codecov[bot]

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

susnux avatar Nov 25 '24 10:11 susnux

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

skjnldsv avatar Dec 05 '24 19:12 skjnldsv

Done @susnux ! Updated :rocket:

skjnldsv avatar Nov 26 '25 16:11 skjnldsv