partials/layouts collections discard dot namespaces in view keys #948
Comments
|
@mootari Thanks for the issue! If you're reporting a bug, please be sure to include:
If your issue is related to one of the following, please open an issue there:
|
|
Hi @mootari can you show an example of how you're actually setting the templates on the collection? There are existing ways to preserve the keys depending on how you're doing it. edit: I see that you're using globs, but I' unclear on how dots in the name are being expanded |
|
Hi, thanks for the insanely quick response! 😄 I'm adding the templates using the following line: this.app.partials(pathLib.join(__dirname, 'templates/partials/**/*.hbs'));I noticed the issue after I added two new templates:
When referencing via |
|
Strange, I think this might be a different issue than suspected. I'm betting it has to do with how the extname is handled somewhere (in templates or either built-in |
|
for discussion, this obviously shouldn't happen just from default node.js path parsing: const path = require('path');
const stem = name => path.basename(name, path.extname(name));
console.log(stem('theme/templates/partials/tocdropdown.item.hbs')) //=> 'tocdropdown.item'
console.log(stem('theme/templates/partials/tocdropdown.hbs')) //=> 'tocdropdown'which makes me think we wrote some code that's causing this somewhere |
|
Unfortunately I had to overwrite renameKey on the collection itself: this.app.partials().options.renameKey = p => pathLib.basename(p, '.hbs');because just passing it to the collection like this: this.app.partials(pathLib.join(__dirname, 'templates/partials/**/*.hbs'), {
renameKey: p => pathLib.basename(p, '.hbs')
});would still ultimately invoke the original callback on the collection. |
Hmm, that seems like a bug. I'd love to get some failing unit tests together for an isolated minimum test case. It would be great if you could even do that locally then paste the unit tests back into a comment here, or do a PR, whatever works for you |
|
Basic example: const app = require('assemble')();
app.task('default', () => {
app.partials('foo', {content: 'foo'});
app.partials('foo.bar', {
content: 'bar',
renameKey: key => key
});
const views = app.partials().views;
if(!views['foo.bar']) {
// ...
}
});
module.exports = app; |
|
Sorry, forgot the renameKey (I've updated the above example). Although arguably an explicitely set key should not be modified either by default. |
agreed! |
|
Hmm, I'm not able to reproduce the issue. Here is what I did: // in "foo.js"
const app = require('assemble')();
// create collections
app.create('partials', { viewType: 'partial' });
app.create('pages');
// add views to "partials" collection
app.partial('foo.bar', { content: '"foo.bar"' });
app.partial('foo', { content: '"foo"' });
// add views to "pages" collection
app.page('aaa', { content: 'Partial: {{> foo.bar }}'});
app.page('bbb', { content: 'Partial: {{> foo }}'});
app.page('ccc', { content: 'Partial: {{partial "foo.bar" }}'});
app.page('ddd', { content: 'Partial: {{partial "foo" }}'});
// check the given collection's "views" cache
if (app.partials.views['foo.bar']) {
console.log('success: foo.bar');
}
if (app.partials.views['foo']) {
console.log('success: foo');
}
// check the app's "views" cache for the given collection
if (app.views.partials['foo.bar']) {
console.log('success: foo.bar');
}
if (app.views.partials['foo']) {
console.log('success: foo');
}
// use a lookup function on the collection
if (app.partials.getView('foo.bar')) {
console.log('success: foo.bar');
}
if (app.partials.getView('foo')) {
console.log('success: foo');
}
// use a lookup function on "app"
if (app.find('foo.bar')) {
console.log('success: foo.bar');
}
if (app.find('foo')) {
console.log('success: foo');
}
app.toStream('pages')
.pipe(app.renderFile('hbs'))
.on('error', console.log)
.on('data', file => console.log(file.content))
.on('end', () => console.log('done'))Then after running |
|
bah, lol. Well I figured out the issue (kind of). I forgot those collections are built-in with assemble, so by re-creating them I "solved" the issue. But that tells us that there is something buggy in one of those built-in collection configs. |
Perhaps the default renameKey callbacks defined in |
|
This seems to be where the action happens: https://github.com/jonschlinkert/load-templates/blob/master/index.js#L115-L119 |
|
I just saw these notifications. From the link that @mootari just posted, I have an idea: |
Whatever it is, is being triggered or caused by the configuration settings in assemble. In |
|
meaning that as @mootari indicated in an earlier comment, the
|
|
|
To clarify, the first call uses the options that got passed with the call to .partials(), the second doesn't. |
|
Right, to also clarify, I think that although there is loading happening, it's how the
|
|
Here's a partial trace to the second call (renameKey is not set on the item options, defaulting to the collection): |
|
while we're on this topic, @mootari and @doowb how do you feel about us changing This would also change the interface in important ways. You would no longer be able to directly get views from the object: Thoughts? |
|
I think changing them to |
I love maps, but we might need to sugar them a bit. Using Array.from() to convert iterators can get old fast. One thing to keep in mind though is that Map keys can be anything. That can be a blessing or a curse.
Yes, please! Anything that cleans up the exposed interfaces is a welcome change in my book. :) |
agreed!
Also agreed! here is what I've done so far to make them easier to work with for our use case 'use strict';
module.exports = class Hash extends Map {
get length() {
return this.size;
}
find(fn) {
for (const [key, value] of this) {
if (fn(value, key) === true) {
return value;
}
}
}
filter(fn) {
const hash = {};
for (const [key, value] of this) {
if (fn(value, key) === true) {
hash[key] = value;
}
}
return hash;
}
map(fn) {
const hash = {};
for (const [key, value] of this) {
const args = fn ? fn(value, key) : [key, value];
if (!Array.isArray(args)) {
throw new SyntaxError('expected .map to return an array: [key, value]')
}
hash[args[0]] = args[1];
}
return hash;
}
reduce(fn, init) {
const hash = {};
for (const [key, value] of this) {
fn(hash, value, key, this);
}
return hash;
}
};edit: in the collection class, we would do: this.views = new Hash(); |
|
there might be better ways to do those things, I'm just working on a PoC for the next version. feedback welcome and appreciated |
|
I like the |
I would just remove the methods then. IMHO the only time I'd be calling them is when I need an object for rendering or context. edit: they were originally doing what you're saying, until I actually needed to use them and had to convert them to an object every time. edit2: fwiw, @mootari and I both just commented on how "Using Array.from() to convert iterators can get old fast." lol. Guess you missed that part. |
|
Oh, I see. that makes sense. |
|
here is class I created for lists: class List extends Set {
get items() {
return this.slice();
}
get length() {
return this.size;
}
slice(fn) {
return Array.from(this);
}
find(fn) {
return Array.from(this).find(fn);
}
map(fn) {
return Array.from(this).map(fn);
}
reduce(fn, init) {
return Array.from(this).reduce(fn, init);
}
filter(fn) {
return Array.from(this).filter(fn);
}
intersection(set) {
return this.filter(ele => set.has(ele));
}
difference(set) {
return this.filter(ele => !set.has(ele));
}
union(...sets) {
const list = new List(this);
for (let set of sets) set.forEach(ele => list.add(ele));
return Array.from(list);
}
} |
Is there a reason for the strict comparison?
I'm a bit torn on this one. I'm not sure if it's a good idea to mimic part of the Array interface and make people believe they're actually dealing with one. Perhaps it's better to just expose .toArray() and only keep the "non-standard" methods. |
I'm not sure what the value of this would be. To convert a set to an array, you just do
We can easily solve your internal conflict over method name semantics by returning a Maybe I'm missing something, but this seems like a non-magical thing that devs would get right away.
We can easily do what was mentioned above and return a new Map or Set, then change that language to "convenience methods are exposed for making the Set/Map easier to work with". But then, I'm really not sure what the value of the methods would be, or of even having Map and Set, since everywhere they're used we'll need to convert them to objects or arrays (for starters: streams/pipeline, middleware, rendering, inside helpers, for looping over collections in templates, merging partials onto the context). The other alternative (which I'm leaning towards) is to just make these private variables and only expose plain objects and arrays so we don't need to worry about confusing devs. |
No need for snark, I'm just trying to give an outside perspective as someone who spent some time digging and stepping through the sources to discover what can or can't be done. Assemble users appear to cover a wide range of proficiency, and not everyone has their debugger set up and ready to go. "If it quacks like a duck ..." etc.
Sounds good to me. Btw, I'm really not trying to bikeshed here but to provide some value with my feedback. If you think I'm being anal let me know and I'll try to dial it back. |
I was just joking! sorry humor comes across poorly here. I have a dry sense of humor edit: the first part was joking, this next part I was not being snarky or funny, just stating my view
I was speaking to your comment: "I'm not sure if it's a good idea to mimic part of the Array interface and make people believe they're actually dealing with one.", which sounds pretty clear that you were saying we would be doing "magic" when you say "making people believe".
Not at all, it's good feeback. I hate the bikeshedding meme since it causes exactly the kind of second-guessing that you're indicating. Please feel free to keep giving us your feedback, it's valuable |
Nah, more like "emulating". I count myself among the group of people that see stuff like .map(), .filter(), .length and immediately think "ah, it's an array, so I can also use .every(), .some(), .concat() and access indices". And then they can't. Might just be a case of RTFM, but it might also hurt first experiences because you have to stay aware of which of those methods are actually provided. |
|
I meant to reply earlier, but I was away from my computer... After @jonschlinkert's comment about returning an object from the
This is how I see it... if we document the api for We've tried to make classes for Collection and List and said, "Let's just use helpers to interact with the methods on them." which started making the templates look like a mess, or ended up forcing us to make helpers that repeated a lot of the same code just to get an object or an array. For this reason, I vote on the classes that inherit from Map and Set but have methods that can easily be used to build objects and arrays that can be added to the context for rendering templates. This will let users use regular helpers that would normally be used with objects and arrays. I hope all of this makes sense 😀 |
version
0.24.3
description
list.hbslist.item.hbspartialsorlayoutscollections using a glob string.renameKeycallback,view.keyis already sanitized to only contain the template basename.renameKeycallback gets invoked andview.keystill contains ".", any remainder after (and including) the dot will be discarded.Source:
assemble/index.js
Lines 140 to 142 in d385ca8
Suggested solution
Warn or error out if adding a view onto a collection replaces an already existing view. Optionally suggest a custom
renameKeycallback.This will also help with situations where users have identical basenames or identical filenames in different directories.
The text was updated successfully, but these errors were encountered: