Cleanup module: block, stack ...

[DivadNojnarg]

will fix #25

[codecov]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (debefdf) 73.25% compared to head (7fd8084) 78.86%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
R/server.R	92.10%	6 Missing ⚠️
R/field.R	87.50%	2 Missing ⚠️
R/mutate-block.R	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
+ Coverage   73.25%   78.86%   +5.61%     
==========================================
  Files          16       16              
  Lines        1701     1708       +7     
==========================================
+ Hits         1246     1347     +101     
+ Misses        455      361      -94     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[DivadNojnarg]

TO DO:

• more test for stack (shinytest2 not included in the coverage percentage ...)

[DivadNojnarg]

CICD and local tests are not reproducible ...

[DivadNojnarg]

This one is good for review @nbenn. I need it for #124.

[nbenn]

What I don't fully understand is why we need block-level server logic to remove a block. Removal of a block to me is an operation applied to a stack. I'm fine with tacking this in a separate PR, if merging this is blocking you in another place. I just feel that changing the return type of the block module is a big change given that ideally blocks themselves are not really involved in this process. What do you think?

[nbenn]

You're changing the return value of block modules from something that is fairly straightforward (the data) to something that is less straightforward. Keep in mind that this involves the block-implementer API, i.e. when a block implementer writes a custom server function, they have to include this logic in their function as well. What happens if this is not done properly? Could we maybe handle this somehow outside of the block server module?

[DivadNojnarg]

@nbenn : what we currently do

graph TD;
    subgraph stack
        subgraph block1;
            remove[Remove] -->|triggers| cleanup_internal[Internal block cleanup: input, output, obs];
            cleanup_internal --> allow_remove[Remove allowed];
        end
        allow_remove ---> callback[Stack 'remove' callback]
        callback ---x |Remove from stack| block1
        subgraph block2
            remove2[Remove]
        end
        remove2 ---> callback
        callback ---x |Remove from stack| block2
    end

Loading

If we want to only return data, we have to move the remove block button to the stack level, that is reworking the stack ui and block ui elements as well as the related JS code @JohnCoene

[DivadNojnarg]

NOTE: We'll have the same problem at the workspace level, where the stack control UI (remove, ...) belongs to the stack and not the workspace.

[nbenn]

@DivadNojnarg From your drawing, what a block would need to provide to more cleanly separate tasks/responsibilities is a "cleanup" function. Then the stack can perform the removal and call the block cleanup code (if that is available).

What we're still laking a bit: things such as the block removal UI should ideally be provided by the stack and somehow "injected" into the block instead of each block having to provide this on its own.

[nbenn]

This prevents removing the first block if there are more than one blocks in a stack. In principle, this is the same problem we have everywhere: removing a block might cause downstream issues if that block contributed to the "necessary" structure of the data. I don't think removal of the first block hast to be handled separately, if we have properly figured out how to handle this "the data is no longer compatible with (some) blocks" scenario.

[DivadNojnarg]

We don't have such solution yet and we have to propose a temporary one. This code can easily be removed when we find out a better way.

Side note: I am actually not even sure if we should allow to remove the first block at all. Is there a case where the first block of a stack would not be a data_block? (For instance, add_block ensures that when the stack has 0 blocks, we can only include a data_block).

[DivadNojnarg]

What I don't fully understand is why we need block-level server logic to remove a block. Removal of a block to me is an operation applied to a stack. I'm fine with tacking this in a separate PR, if merging this is blocking you in another place. I just feel that changing the return type of the block module is a big change given that ideally blocks themselves are not really involved in this process. What do you think?

Yes that's a good point.

Blocks are modules (like the stack). The main issue with shiny is when you remove a UI element, the server part isn't:

• shiny inputs remain (Request: allow deleting an input that was removed using removeUI() rstudio/shiny#2374). No clean method so far.
• All observers remain. This can be destroyed.
• Outputs remain (Can shinyoutput entries be removed? rstudio/shiny#1989). This has been partially solved.

As an example, in the workspace PR #124, I am facing issues because I am not doing this cleanup which totally corrupts the server part when I start to remove/add stacks (some inputs are still considered by observers while they should not).

It is easier to do this cleanup from the module (block) itself. If we wanted to do it from the stack, we would need to store all of the block server infrastructure (observers, ...) in a place where we can delete them. This is likely possible but I am not sure about the gain.

The last point is that each remove button is part of the block UI (like collapse, show code, ...). When we click on remove, we must return it's value to the stack, which is why I added an extra entry to the returned value. Without doing this, there is no way for the stack to know about what happens in a block. This could definitely change if we considered that the stack would be responsible from handling the current block toolbar UI and the block UI would be just the code/data output and fields. This might break what @JohnCoene did on the JS side. I am open to discuss this further.

Maybe @JohnCoene you have ideas here.

[DivadNojnarg]

cc @nbenn.

#132 and #119 overlap:

• Handle "remove" button namespace to stack level + ui tweaks #132 handle the basic UI part and block removal from the stack server function.
• Cleanup module: block, stack ... #119 will propose a better cleanup solution for input/output and observers of the removed modules.