Sanitize basePath

[madeleineostoja]

The basePath argument to SimplaPaths.observe() is confusing - intuitively you specify a leading slash, like /base/path, but that results in the following //base/path which is invalid. Likewise I was just guessing whether I should include a trailing slash or not, if I had it would have resulted in /base/path//sid which is also invalid.

I think the fix for this is for observe() just to sanitize whatever it gets in basePath (within limits), particular in regard to leading and trailing slashes.

[madeleineostoja]

As an aside - paths like //path/to/content should warn, not error in console. Common for eg: dynamically defined SIDs (eg: headings on simpla.io) to have momentary missing parts of a path.

[bedeoverend]

Yeah perhaps it would make more sense if we treated paths as normal file paths in terms of what we'll accept and automatically figure out? e.g. We know that //foo == /foo but foo != /foo and /foo//bar == /foo/bar and maybe /foo/bar/ == /foo/bar? In which case we should map out exactly how we're normalize it - or just follow a standard e.g. Something like node's normalize method.

[madeleineostoja]

I think a general solution to this is dangerous and would easily result in incorrect behavior. Just the input to observe should be normalized because the argument is ambiguous. See comments on other issue re throwing vs warning on invalid paths.

[bedeoverend]

If we just update the docs so that the default value is actually '' (given that's what it should be, not /) - would the confusion still be there? An example of then using it would be:

Simpla.observe(document.querySelector('about-page'), '/about');

Actually I just realised, this might just be a bug, I may have misread your initial comment - you did:

Simpla.observe(element, '/base/path');

which failed? That should have passed, that's a bug. If that did succeed, would that less confusion?

[madeleineostoja]

Yes that failed, resulted in double leading slash. In any case don't see what the default value has to do with it (and surely that should be /, otherwise it'd fail if not defined no?).

That's intuitive enough, requiring a leading slash, as per normal paths, but the end of it is a little less clear, because you could define / or /base, which aren't consistent. Or can you not define /? That's confusing.

[bedeoverend]

So you can't give it /, but you can give it /base. It's probably easier if you think of it as a prefix. So for whatever path is on an element, add the prefix: ${prefix}/foo/bar. If you give it /: //foo/bar, if you give it '': /foo/bar, /base: /base/foo/bar

Anyway, I get what you mean about it being confusing. Personally, I still think normalizing across the board would probably make more sense. So you could do Simpla.get('//foo/bar') and it would accept it no worries, like you would if you were dealing with file paths - makes it much easier to GROK, what do you think?

[madeleineostoja]

I vote hard no on normalizing across the board for the reasons I mentioned above - /foo/bar and /{temporarily-undefined}/foo/bar are very different paths, and it's very possible that it would fetch completely unrelated data that would either a) result in hella cryptic errors/bugs, or b) give you nasty FOUC. I'd prefer the current behavior (throwing) to that.

I don't see the problem with sanitizing the basePath argument here though? What's the issue with making it less of a PITA to guess by just checking what we were given? There's no ambiguity or edge cases with this - if someone gives it /foo/bar/ they obviously mean it to become /foo/bar/baz, not /foo/bar//baz. Same as /, that should obviously become /foo/bar, not //foo/bar. That last one in particular is a big gotcha for no reason. To be explicit in a function call it makes sense to give basePath something, and that something should be /, not '' (to be consistent with paths everywhere else). It's just really confusing, and requires users to understand the internals of what this function is doing.

[bedeoverend]

FWIW I think that the user should check something's undefined before handing it to the element - otherwise I would expect it to get the wrong value. But, erroring is also OK IMO - so happy to stick with that.

As for that, yeah I definitely would like to normalize, I'm just worried it'll be inconsistent elsewhere. But, I've just realised I've been thinking of it wrong: this is actually the only place where you give a path and it will be joined with another path else i.e. all other times you're dealing with a path it's a full one, and then at other times it's just sid and gid which aren't allowed / in them.

Given that, yup happy to sanitize base path.

[madeleineostoja]

This is actually the only place where you give a path and it will be joined with another path else i.e. all other times you're dealing with a path it's a full one, and then at other times it's just sid and gid which aren't allowed / in them

Yes, exactly. This isn't the same thing as defining a path in elements or get or whatever, it's a totally separate argument, and therefore can play by different rules. It's actually more consistent with other places paths are used by normalizing, because it's weird af for a, er, 'base' path to be '' rather than /.

[madeleineostoja]

and

FWIW I think that the user should check something's undefined before handing it to the element - otherwise I would expect it to get the wrong value

Yeah that's fair enough - but yeah, I'd rather it blow up explicitly (or warn explicitly, but that's neither here nor there ;-) in that case rather than do the javascript thing of trying to make everyone happy and breaking in much weirder ways

Though one thing about the user checking if something is undefined before giving it to the element - that breaks down when you have any kind of templating - eg:

<simpla-text path="/[[myProp]]/rest/of/path"></simpla-text>

That will result in //rest/of/path until myProp is defined. The only way around this is to say "well, don't use templating for paths", which sounds like a pretty shitty answer.