Dataset.key() docs + proposal for simpler API

[ryanseys]

There were no docs at all for Dataset.key() so I added some here. They are not perfect though because the weird param structure used for the function.

Currently the params are something like either:

Name	Type
options	object
options.namespace	string
options.path	array

or

Name	Type
path	any type, passed in as arguments list

Issues:

1. That's really hard to document... I don't even know how to do it here, let alone in valid JSDoc (don't think it's possible, think you need something like a @paramset: what is suggested here)
2. That's really hard to understand, even if documented.
3. If I have a path array, only option I have is use the first option and know that this.namespace exists or manually provide it, or I can use Function.apply to apply the array to the function as arguments (yikes!)

Proposal (not implemented here):

Just accept an object, specifying path. It's not hard to understand, document and requires only 9 characters extra for the developer to surround their old arguments for this new method:

.key({ path: [ <old arguments> ] })

Name	Type	Optional
options	object	false
options.namespace	string	true (use this.namespace if not provided)
options.path	array	false

[stephenplusplus]

Can you change the named parameter to options as well? It is still keyConfig.

Here's an example of how we can document our current structure:

But, I'm not a big fan of the variable arity, either. It's not something I've seen too often in js-land, since we have the ease of passing around configuration objects.

I'm not sure you'll like this any better, but I propose we instead only allow a single argument to key, but it can be of three types:

// path stringdataset.key('Company');// path arraydataset.key(['Company',1]);// object specifying namespace and pathdataset.key({namespace: 'MyNS',path: ['Company',1]});

So basically, if provide an object: control all components. Provide an array or string, you're providing the path. I think all it takes is seeing an example, reading about the namespace inheritance, and it's pretty straightforward from there.

[ryanseys]

Yeah, I think that'll give us a good compromise. I'll work on making that happen.

[ryanseys]

PTAL. This should change .key('kind', id) to .key(['kind', id]).

[stephenplusplus]

If there isn't something that covers this already, could you add a test on dataset.key to see if it handles strings as expected?

[ryanseys]

Agh! Good call, I found an issue. toArray() should be arrayize().

Question now though, is: dataset.key('Company'); an incomplete key equivalent to dataset.key(['Company', null]);? (From your proposal)

[ryanseys]

PTAL

[stephenplusplus]

My dataset.key() example was I guess foreshadowing our desired support for auto-injecting null. Since we don't have that yet, and even after we do, this would be an invalid key currently. After we auto inject null (if we do) it would be a valid, incomplete key.

[ryanseys]

Yeah, I just realized that. Although our code doesn't do it anywhere, it would create an invalid key. I will get started on auto-injecting nulls if we think that's a good idea? Let me know.

[stephenplusplus]

I think it's safe to do that. We used to take an array and if it wasn't an even number length, we used the first item as the namespace. Back then, it wasn't possible for us to infer an odd length meant a key didn't have an id or if it didn't have a namespace, so we decided to require nulls for ids. Now that we require explicit specification of a namespace, we can safely use the logic above to pluck off the id from the other end.

Ps: I'm not sure the above story is true, but I think it makes sense :)

[ryanseys]

That was going to be my approach as well, just if keyPath.length % 2 === 1 ---> inject null

[ryanseys]

Backward compatibility fail: ds.key('Kind', 123) will now result in an incomplete key. Is this okay?

[ryanseys]

... In theory we could support it if we check if arguments.length > 1

[stephenplusplus]

Yeah, as long as we bump the major. But either way, semver grants us free reign to do whatever pre-1.0.

[stephenplusplus]

While we are still pre a major release, I vote to rid it instead of supporting it forever.

[stephenplusplus]

• * // Note: id is used for numeric identifiers and name is used otherwise

Can you remove that line? While it's informative for us, it's probably not relevant to the doc reader.

[ryanseys]

The line above it states that name='Google' so I thought I should clarify. Also in the docs it says:

• An identifier for the individual entity, which can be either
  • a key name string
  • an integer numeric ID

[ryanseys]

But I'll remove it.

[stephenplusplus]

I actually like it now. I didn't notice the difference in your example explaining name= vs id=. Makes sense to keep it 👍

[ryanseys]

So we support:

varkey=ds.key('world');// orvarkey=ds.key(['world']);// orvarkey=ds.key({path: ['world']});

Should we support:

varkey=ds.key({path: 'world'});

?

[ryanseys]

I think yes unless there's any objections.

[stephenplusplus]

I do see how if we support a string in one place, we should support it in another. However, I think we can get away with requiring an array there, because an object is the explicit specification format. We should expect them to conform to the schema, where a namespace is a string, and a path is an array.

I wanted to allow ds.key('world') as a shorthand, so that we didn't force the user to make an array for a single item as the only argument passed to our function. ds.key(['world']) feels like a quirk of our API that the user shouldn't have to worry about.

Just my opinion.

[ryanseys]

Yeah, we should probably be more explicit with the values we accept. I agree with you.

We can document it as:

varkey=ds.key(kind);// kind is a string e.g. 'Company'// orvarkey=ds.key(path);// path is an array e.g. ['Company'] or ['Company', 123]// orvarkey=ds.key({namespace: namespace,path: path});

[ryanseys]

So, I actually didn't need to do much in order to support this. It seems there was only a check making sure that we didn't pass in less than two values for the path. I just changed it to make sure we were passing in at least 1 value for the path. Updated the docs and tests to reflect this change.