-
Notifications
You must be signed in to change notification settings - Fork 2k
embed views and load them using templateCache #899
Conversation
@andreafdaf Are the tests running successfully on your local dev? It appears that the client-side tests are failing. |
@@ -34,7 +34,7 @@ module.exports = { | |||
'modules/*/client/*.js', | |||
'modules/*/client/**/*.js' | |||
], | |||
views: ['modules/*/client/views/**/*.html'], | |||
views: ['modules/*/client/**/*client.view.html'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to make it a little more flexible, for example... if I have something like "...moduleName/client/directives" and I put templates and js files under the "directives" folder with the old rule they would not be found
of course one can put js from directives under js and html under views, but it makes or doesn't make sense based on just a programmer point of view... so that's why it was changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the best practice in the cases of where to put the html templates for directives.. but I usually put them in my views folder. Perhaps, someone else has some insight into that. But generally, I'd be weary of making changes to a core feature like the assets locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you're right... maybe 'modules//client/**/.html' like is done for 'modules//client/**/.js' would be the most flexible one (and wouldn't break compatibility with code coming from an earlier version)
just to explain my point a little bit more:
- if a file is under a "client" folder and under a "views" folder why do we have to call it "viewName.client.view.html" while it could be simply "/client//views/viewName.html"
- if I create a folder "templates" to put html templates, should I call them "fileName.client.template.html"?
cannot I simply call all my views "viewName.client.view.html" and put them in the folder I want, since with templateCache IDs I know that no matter what this view is going to be "viewName"?
@andreafdaf I don't know how this implementation of template cache is working. Could you explain it a bit? Also, could the client-side tests be failing because the route configurations have changed? Also, the change of the assets client views definition might be related. |
hi @mleanos sorry but I didn't tested (hehe ._.) since it was working on browser and is a mod taken from a project already running in production so... I guess that the tests must be rewritten in some small part, tests fail in all browsers but views are correctly loaded and cached so it's just a test inner thing (template IDs probably), I guess I found a solution but have no time to work on it right now anyway it works like that:
some performance gain and nothing changes in development (just calling templates by ID instead of by URL, that's it) |
ok tests now pass, it was a simple mod (memo to myself: read contributing guidelines more carefully) anyway... this PR is just a starting point to discuss about this feature, this same result can be achieved in multiple ways so now it's up to the community to decide if it fits or not, must be changed or what ✌️ btw, I'm sorry to see that "Coverage decreased (-0.4%) to 51.842%"... how can I improve it? |
@andreafdaf I'm not exactly sure about the code coverage. We could use some more eyes on this one. @codydaig @ilanbiala @lirantal @rhutchison @trainerbill @simison |
} | ||
}); | ||
} | ||
async.concat(res.app.locals.htmlFiles, readViewFromDisk, function (err, embeddableViews) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to concat the files if they are being loaded in individually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you've got your point 👍
@andreafdaf Im not completely familiar with ng-template so i out a couple inline questions above. Ill have some time later today to test and play. I'll touch base afterwards and give you more through feedback. |
@andreafdaf So what's the difference between your approach and an approach using a gulp task like this: https://www.npmjs.com/package/gulp-templatecache? That gulp task is currently implemented in our gulp file so I was wondering what the difference was, and what the benefits of one over the other are? |
@andreafdaf So the issue with putting everything inside the index.html page is that the index.html page cannot be cached because it injects the user object directly into the page. But compiling the templates into a file that get's put into the /public folder allows for production environments to cache the html files, as well as allow nginx (or similar) to server the files instead of relying on express, which is usually he preferred method in production versus relying on express. |
@codydaig I was writing something else but then you comment popped up! so what about also using https://www.npmjs.com/package/grunt-angular-templates , so that developers can have this same functionality no matter which task runner they choose to use? |
@andreafdaf Currently we're switching over to gulp so, I think it's fine leaving support for this on the gulp side. I apologize for the initial link, the this is the one we're actually using. I forgot we did a name swap in the plugins. |
@codydaig dropping grunt? good news! I thought that (in this project) gulp was behind grunt, but you are clearly showing me that there are some tasks that are used only in gulp so I guess we can close this! |
@andreafdaf Gulp isn't fully ahead of grunt yet. But in the near future it will be. |
Ref: #853 |
@andreafdaf I've felt it's okay to use the file path as an ID even with templateCache, perhaps just without .html at the end to indicate it's an inline template. Makes it easier to find them. |
@simison since templateCache is only used in production you were totally right (imho), if development needs the path to find the views (so we put the whole path when we call a templateUrl) then in production the IDs from templateCache should be the same as the path if we don't want to write more code to make this distinction happen (when in production call this ID, when in development call this path) |
this is where I proposed to embed views and load them using templateCache