Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Page variable is overwritten when page.js.twig is included in a dynamically loaded component #1104

Open
alexweissman opened this issue Jul 23, 2020 · 5 comments
Labels
confirmed bug Something isn't working frontend The frontend interface needs discussion A decision needs to be taken by the dev team
Milestone

Comments

@alexweissman
Copy link
Member

page.js.twig currently sets the page Javascript variable to import page-specific variables from the server into the client JS environment. However it looks like in some cases we load this partial both in the base page as well as dynamic components such as modal forms.

This means that the original value set in the base page gets completely overwritten when the modal form is loaded.

The simplest solution seems to be merging, rather than setting, the page variable by changing page.js.twig:

{# Page variables needed by client-side code (Javascript). #}
{% autoescape 'js' %}
    if (typeof page === 'undefined') {
      var page = {};
    }

    page = $.extend(
        true,               // deep extend
        {{ page | json_encode(constant('JSON_PRETTY_PRINT')) | raw }},
        page
    );
{% endautoescape %}

This doesn't completely guarantee that nested values in page won't be overwritten, of course. But it at least provides a mechanism for easily setting additional client-side variables while keeping them scoped under the top-level page object.

@alexweissman alexweissman added confirmed bug Something isn't working frontend The frontend interface labels Jul 23, 2020
@alexweissman alexweissman added this to the 4.4.x milestone Nov 24, 2020
@alexweissman
Copy link
Member Author

We could also consider a system that automatically scopes variables in dynamically loaded components to those specific components, a la Angular and other JS frameworks. Now that I think about it, blindly doing a deep merge doesn't make that much more sense than blindly doing an overwrite.

@alexweissman alexweissman added the needs discussion A decision needs to be taken by the dev team label Nov 25, 2020
@Silic0nS0ldier
Copy link
Member

Silic0nS0ldier commented Nov 25, 2020

IMO the page variable should not be treated as immutable in JS. My thoughts are to reign in scope of the proposed change to conditionally set it.

@alexweissman
Copy link
Member Author

I agree, but the way we have it designed now it is too easy to accidentally overwrite it by including page.js.twig more than once. A lot of that honestly is my fault in the way I set up the base modal template, which reuses the page variable to communicate validation rules to modal forms.

@Silic0nS0ldier
Copy link
Member

Ugh. I spotted a type.

I meant it should be treated as immutable in JS.

@alexweissman
Copy link
Member Author

Ah ok. Well in that case we definitely should be using separate variables for different components, then.

@lcharette lcharette modified the milestones: 4.6.x, 5.0.1 Nov 25, 2023
@lcharette lcharette modified the milestones: 5.0.1, 6.0.0 Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed bug Something isn't working frontend The frontend interface needs discussion A decision needs to be taken by the dev team
Projects
Status: References
Development

No branches or pull requests

3 participants