GPII-3698 Work in progress #28

sgithens · 2019-05-09T18:33:45Z

@the-t-in-rtf This is my WIP of what has been needed to fixup the messageHelper for use in both the server and client side in a full gpii-handlebars based project (gpii-devpmt)

Moved the client side to read bundles from the model. I think we mentioned this should likely happen for the node.js side as well.
There is an issue with the best way to configure the templateManager to use the correct aware grade.
It's a little wonky how the grade inheritance works with deciding when to render... I changed things on the client so that the rendering always happens when a new common event allResourcesLoaded is finished. If there are any more future grades that need to read another type of resource (templates, bundles, datafiles... ) before rendering, I think they should all have them wait on firing this common event for all future grades.

- Changing the event that we render on to be onAllResourcesLoaded rather than onTemplatesLoaded so all implementations can render on the same event.

…from options area.

src/js/common/messageHelper.js

the-t-in-rtf · 2019-05-21T07:52:48Z

src/js/client/renderer.js

+            "onTemplatesLoaded": null,
+            "onAllResourcesLoaded": {
+                events: {
+                    // onMessagesLoaded: "onMessagesLoaded",


You need to explain why you commented onMessagesLoaded out. If there' s a new mechanism that ensures that rendering can take place after messages are loaded, fine, then it should be removed. If it's here for some other reason, you need a TODO or comment.

the-t-in-rtf · 2019-05-21T07:54:44Z

src/js/client/renderer.js

+            "onTemplatesLoaded": null,
+            "onAllResourcesLoaded": {
+                events: {
+                    // onMessagesLoaded: "onMessagesLoaded",


This is a bit baffling, you still have the rollup event, and you still launch the template retrieval, but you've commented out the templatesLoaded event twice?

the-t-in-rtf · 2019-05-21T07:55:27Z

src/js/client/templateAware.js

@@ -130,7 +130,7 @@
                type: "gpii.handlebars.renderer.serverAware",
                options: {
                    listeners: {
-                        "onTemplatesLoaded.renderMarkup": {
+                        "onAllResourcesLoaded.renderMarkup": {


I think the rollup is a good improvement and will help once we figure out what's going on with template loading.

Just to clarify, I mean that if we choose to support an event-based approach, a rollup is an improvement.

the-t-in-rtf · 2019-05-21T07:59:03Z

src/js/client/templateManager.js

-                type: "gpii.handlebars.renderer.serverAware"
+                // TODO sgithens This needs to be updated depending on the app from serverAware to
+                // serverMessageAware dynamically.
+                type: "gpii.handlebars.renderer.serverMessageAware"


You could argue that we could put comments next to any type when there are variants, but that puts the burden on implementers to know which code to read and to read it. If this switch is required often enough, maybe a mix-in grade that does this would help? i.e. a templateManager.serverMessageAware grade that is also mentioned in the docs and used in tests?

the-t-in-rtf · 2019-05-21T08:03:51Z

src/js/common/messageHelper.js

-            var messages = fluid.get(rootContext || dataOrRootContext, "data.root.messages");
-
+            var messages;
+            if (serverAware) {


We should have another meeting with @amb26 during our overlap, this is getting a bit out of hand and it's largely my fault. I started this "server aware" pattern long ago, and it seems wrong headed for the long term. I think we'd be better off migrating to something like named global variables for message bundles, templates, et cetera, or at least using something standard like a resource loader. We should also at least figure out how/if we can avoid mixing events and model changes as we do currently.

the-t-in-rtf · 2019-05-21T08:04:43Z

src/js/common/messageHelper.js

            // If we have a third argument, then the second argument is our "data".  Otherwise, we use the root context (equivalent to passing "." as the variable).
-            var data = rootContext ? dataOrRootContext : fluid.get(dataOrRootContext, "data.root");
+            // TODO var data = rootContext ? dataOrRootContext : fluid.get(dataOrRootContext, "data.root");


OK, this is another case where you've neutered a feature without any comment on why, and left the old functionality commented out. What's your thinking here?

the-t-in-rtf · 2019-05-21T08:06:16Z

src/js/common/messageHelper.js

@@ -35,7 +41,7 @@ fluid.defaults("gpii.handlebars.helper.messageHelper", {
    invokers: {
        "getHelper": {
            "funcName": "gpii.handlebars.helper.messageHelper.getHelper",
-            "args":     ["{that}"]
+            "args":     ["{that}", "{gpii.handlebars.renderer.serverAware}"]


If we do end up needing both "server aware" and "non server aware" in the short term, I'm wondering if we can automatically handle this using context awareness so that people only need to use the single grade.

the-t-in-rtf · 2019-05-21T08:07:48Z

src/js/common/renderer.js

@@ -121,7 +121,8 @@ var Handlebars = Handlebars || {};
        defaultLocale: "en_us",
        defaultLayout: "main",
        events: {
-            onTemplatesLoaded: null
+            onTemplatesLoaded: null,


We should discuss whether there should be an onMessagesLoaded event here as well, however we agree to handle things, the approach should be reflected in the common grade as well.

the-t-in-rtf · 2019-05-21T08:09:27Z

src/js/client/renderer.js

@@ -97,7 +98,13 @@
            }
        },
        events: {
-            "onTemplatesLoaded": null
+            "onTemplatesLoaded": null,
+            "onAllResourcesLoaded": {


If we decide to use async activities to populate templates and messages, one option would be to have a promise chained event that handles the loading and which fires onAllResourcesLoaded as its last step.

the-t-in-rtf · 2019-05-21T08:14:59Z

@sgithens and @amb26, I think I'm starting to understand the intent behind this pull, but I think we should meet to discuss whys and hows a bit.

While we wait, it would be good to at least have an approach that actually passes the associated tests. In its current state, the commented out passing of data to the message helper breaks three tests.

the-t-in-rtf · 2019-05-23T11:42:59Z

See GPII-3698 for details.

gpii-bot · 2019-05-28T12:05:46Z

CI job failed: https://ci.gpii.net/job/gpii-handlebars-tests/1/

the-t-in-rtf · 2019-05-28T16:46:44Z

@sgithens, I have been working to fix the CI config for this project. If you merge with master and commit, you should have everything you need to run tests against your work.

CLAassistant · 2021-08-23T15:50:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

sgithens added 6 commits February 6, 2019 06:08

GPII-3698 WIP to fixup the messageHelper

d03b986

Merge remote-tracking branch 'GPII/master' into GPII-3698

24fe982

GPII-3698 Refactoring event to render on

525b868

- Changing the event that we render on to be onAllResourcesLoaded rather than onTemplatesLoaded so all implementations can render on the same event.

GPII-3698 Continuing fixup of client side i18n

c7dbc0e

GPII-3698 Temporary check for server side renderer to fetch messages …

a07eab2

…from options area.

GPII-3698 Removing extra logging

0ea4e1f

the-t-in-rtf reviewed May 20, 2019

View reviewed changes

src/js/common/messageHelper.js Show resolved Hide resolved

sgithens added 2 commits May 20, 2019 16:45

Merge remote-tracking branch 'GPII/master' into GPII-3698

43944d2

GPII-3698 Minor lint cleanup

1efa27b

the-t-in-rtf reviewed May 21, 2019

View reviewed changes

jobara changed the base branch from master to main October 22, 2020 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPII-3698 Work in progress #28

GPII-3698 Work in progress #28

sgithens commented May 9, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf May 21, 2019

the-t-in-rtf commented May 21, 2019

the-t-in-rtf commented May 23, 2019

gpii-bot commented May 28, 2019

the-t-in-rtf commented May 28, 2019

CLAassistant commented Aug 23, 2021

GPII-3698 Work in progress #28

Are you sure you want to change the base?

GPII-3698 Work in progress #28

Conversation

sgithens commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-t-in-rtf commented May 21, 2019

the-t-in-rtf commented May 23, 2019

gpii-bot commented May 28, 2019

the-t-in-rtf commented May 28, 2019

CLAassistant commented Aug 23, 2021