3.0: Widgets losing references to global `data` and widget context

myovchev commented 3 years ago

Describe the bug

Widgets are losing references to data.page and data.contextOptions on insert/update (edit mode).

To Reproduce

Create test widget:

// modules/title-widget/index.js
module.exports = {
extend: '@apostrophecms/widget-type',
options: {
label: 'Print page title',
var: null
},
fields: {
add: {
  var: {
    type: 'string',
    label: 'A var'
  }
}
}
};

{# modules/title-widget/views/widget.html #}
<h1>Title - {{ data.page.title }}</h1>
<h2>Context Var - {{ data.contextOptions.var }}</h2>
<h2>Options Var - {{ data.options.var }}</h2>
<h2>DB Var - {{ data.widget.var }}</h2>

Integrate it with Apostrophe and add it to any page schema:

module.exports = {
options: {
label: 'Some Page'
},
fields: {
add: {
  main: {
    type: 'area',
    options: {
      widgets: {
        title: {  var: 'Option val'  } // testing options propagation
      }
    }
  }
}
}
};

Pass context in the above page template

{% area data.page, 'main' with {
title: {  var: 'Context val'  }
} %}

Insert the widget, set the A var field to db val. The output of the widget JUST after insertion is:
```
Title -
Context Var -
Options Var - Option val
DB Var - db val
```

Hit Update on the page. The output is:

Title - Home
Context Var - Context val
Options Var - Option val
DB Var - db val

Edit the widget and save with no changes, the output is the same as 4.
Refresh or hit Preview on the page, the output is the same as 5.

Expected behavior

The global data context and the specific widget context should be always available.

Details

Version of Node.js: v12.21.0

Server Operating System: Ubuntu 20.04.2 LTS

Additional context: Tested against latest state of 3.0 branch

boutell commented 3 years ago

This is not a new issue in 3.0. It is definitely an issue we have been discussing for a long time, and we've gone back and forth on the best resolution.

In a past github issue, which I unfortunately cannot find, in which we discussed this at great length and decided that widget templates should have access to data.page, etc. whenever possible, not just when rendering the page template. Specifically, the plan was to re-render the body of the page when saving any widget modal, just as we would re-render it when a piece modal is saved, and for the same reason: the side effects of the new data on the page are not as obvious as they seem.

I like this approach because it meets developer expectations and because it is not a design backwards compatibility break late in the 3.0 development cycle. It is also straightforward to implement.

So I propose we stick to that goal.

However it should be noted that both implicit options like data.page and context options will not be available inside the edit modal itself. This is unavoidable because they can be large or even circular data structures in Nunjucks on the server side and there is no sustainable way to pass them to the modal context. I think however that this should not be too surprising. When a widget is being edited in a modal, it is not on the page, so why would it get the data of the page?

Lack of access to the page is really only surprising when you've just edited and saved an existing or new widget, and that is what this fix will resolve.

abea commented 3 years ago

When a widget is being edited in a modal, it is not on the page, so why would it get the data of the page?

I don't think we should consider that a common expectation. I think devs more expect things to be consistent when the data context hasn't changed.

boutell commented 3 years ago

OK, I'm probably being optimistic then in regarding it as an expectation. However, it is close to impossible to implement it successfully for the "add widget" action, and very messy for the "edit widget" action (one circular reference and you're toast). So I think that is the story we may need to work on telling as well as we can.

boutell commented 3 years ago

To clarify, I can't anticipate the context data that might be passed to a new widget being edited in the modal once "on the page" because it hasn't been on the page yet, and that data will not be known until Nunjucks, not Vue, renders it in a very different situation.

myovchev commented 3 years ago

@boutell @abea As I can see it right now - there is only a refresh problem. I don't expect page.data/data.piece to be available in the modal but when rendered on the page, after one hits 'Save' button. At the point when the widget template itself is rendered on the page it needs the information from the page data context to render the result of that insertion, it might be even area which data belongs to a page/piece. And this works WONDERFULLY in my current project with the refresh exception. I'm guarding when needed (e.g. render area which belongs to the page) with simple if data.page....

You can think of header widget which renders page title, passes page._ancestors to breadcrumb fragment, passes page to infobox area which is defined on every page (because every page extends an abstract page type adding that infobox area definition).

And here this references to contextual only fields - the infobox would be contextual, I don't wanna bother the editor to manage it via the widget modal, but only to setup the header widget options (e.g color, choose predefined shape, etc) and then choose what to do next directly on the page (e.g. use the new area(s) you see to insert any widget I allow you to). It's a painless process for the end user which exactly corresponds to the word building - chained actions, react on what you see on the screen. It's a pleasure to design and put all those pieces together.

I have little to no use of widget context so far. I've given it as example in this issue because it behaves the same as data.page/data.piece. I can see it useful for passing options from another template (less likely to happen with my current architecture) or data down to widgets which is outside of the page.data/page.piece scope.

boutell commented 3 years ago

We have implemented the refresh behavior we were discussing, it has landed in the 3.0 branch. That's where this will stay for now, as it is difficult if not impossible to do the same for the dialog boxes, but it's great having this fixed for the "just inserted into the page" and "just edited and saved back to the page" scenarios.

myovchev commented 3 years ago

I'll switch to the relevant commit hash and I'll give it a try.

boutell commented 3 years ago

Reopening this ticket. It seems to have a race condition that sometimes results in a jarring jump to top of page when using apostrophe's undo button. So we have reverted this change and pushed it back an iteration, but it will still appear soon post 3.0 launch (that is happening today (: )

myovchev commented 3 years ago

I fully understand. I've just officially entered beta (like minute ago) and the tasks in order to enter stable are not fitting the screen (it's a quite big screen :) )

apostrophecms / apostrophe