Skip to content

GPII-3698 Work in progress #28

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

GPII-3698 Work in progress #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d03b986
GPII-3698 WIP to fixup the messageHelper
sgithens Feb 6, 2019
24fe982
Merge remote-tracking branch 'GPII/master' into GPII-3698
sgithens Feb 12, 2019
525b868
GPII-3698 Refactoring event to render on
sgithens Mar 8, 2019
c7dbc0e
GPII-3698 Continuing fixup of client side i18n
sgithens May 9, 2019
a07eab2
GPII-3698 Temporary check for server side renderer to fetch messages …
sgithens May 9, 2019
0ea4e1f
GPII-3698 Removing extra logging
sgithens May 9, 2019
43944d2
Merge remote-tracking branch 'GPII/master' into GPII-3698
sgithens May 20, 2019
1efa27b
GPII-3698 Minor lint cleanup
sgithens May 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/js/client/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@

// A "server aware" grade that depends on being able to communicate with an instance of
// `gpii.handlebars.inlineTemplateBundlingMiddleware`.

fluid.defaults("gpii.handlebars.renderer.serverAware", {
gradeNames: ["gpii.handlebars.renderer"],
templateUrl: "/hbs",
Expand All @@ -97,7 +98,13 @@
}
},
events: {
"onTemplatesLoaded": null
"onTemplatesLoaded": null,
"onAllResourcesLoaded": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

events: {
// onMessagesLoaded: "onMessagesLoaded",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

onTemplatesLoaded: "onTemplatesLoaded"
}
}
},
listeners: {
"onCreate.loadTemplates": {
Expand Down Expand Up @@ -128,7 +135,7 @@
};

fluid.defaults("gpii.handlebars.renderer.serverMessageAware", {
gradeNames: ["gpii.handlebars.renderer.serverAware"],
gradeNames: ["gpii.handlebars.renderer.serverAware", "fluid.modelComponent"],
messageUrl: "/messages",
invokers: {
"cacheMessages": {
Expand Down
2 changes: 1 addition & 1 deletion src/js/client/templateAware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@
type: "gpii.handlebars.renderer.serverAware",
options: {
listeners: {
"onTemplatesLoaded.renderMarkup": {
"onAllResourcesLoaded.renderMarkup": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

func: "{gpii.handlebars.templateAware.serverAware}.renderInitialMarkup"
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/js/client/templateManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@
gradeNames: ["fluid.component"],
components: {
renderer: {
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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

},
// All components that require a renderer should be added as children of the `requireRenderer` component
// to ensure that they are created once the renderer is available.
requireRenderer: {
createOnEvent: "{renderer}.events.onTemplatesLoaded",
createOnEvent: "{renderer}.events.onAllResourcesLoaded",
type: "fluid.component"
}
},
Expand Down
20 changes: 13 additions & 7 deletions src/js/common/messageHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,25 @@ var gpii = fluid.registerNamespace("gpii");

fluid.registerNamespace("gpii.handlebars.helper.messageHelper");

gpii.handlebars.helper.messageHelper.getHelper = function () {
gpii.handlebars.helper.messageHelper.getHelper = function (that, serverAware) {
return function (messageKey, dataOrRootContext, rootContext) {
if (arguments.length < 2) {
fluid.fail("You must call the 'messageHelper' helper with at least a message key.");
}
else {
// Pick up the message bundles from the root context, which is always the last argument.
var messages = fluid.get(rootContext || dataOrRootContext, "data.root.messages");

var messages;
if (serverAware) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

messages = fluid.get(serverAware, "model.messages");
}
else {
// Currently, the server verion of the renderer is still storing the messages in it's
// options area, so if no serverAware is wired in, we'll assume this is running in node.js
messages = fluid.get(rootContext || dataOrRootContext, "data.root.messages");
}
// 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");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

var resolver = fluid.messageResolver({ messageBase: messages });
return resolver.resolve(messageKey, data);
return resolver.resolve(messageKey, {});
}
};
};
Expand All @@ -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}"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
});
3 changes: 2 additions & 1 deletion src/js/common/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ var Handlebars = Handlebars || {};
defaultLocale: "en_us",
defaultLayout: "main",
events: {
onTemplatesLoaded: null
onTemplatesLoaded: null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

onAllResourcesLoaded: null
},
model: {
messages: {},
Expand Down