-
Notifications
You must be signed in to change notification settings - Fork 68
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.
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
Refactor API signatures #37
Comments
On Jun 1, 2015 1:10 PM, "Dave Jensen" notifications@github.com wrote:
It can more correctly be thought of as vehicle.dataInterface.method.
Members were concretely defined to avoid magic strings.
This is a feature, not a bug. The idea was to take advantage of
|
One other reason to keep it like it is is that development becomes easier
|
But now all of your code must check both the availability api, check for null, and handle an error in the callback. |
Is it "magic" if it is defined in documentation and specification? |
On Jun 1, 2015 5:32 PM, "Dave Jensen" notifications@github.com wrote:
Initially, we had all attributes on vehicle as non-nullable. I don't know
|
@djensen47 Thanks for raising this issue. I fully agree with making unified APIs for accessing vehicle information than separate APIs. I think we could have the benefits like
[1] http://www.w3.org/2009/dap/ |
On Jun 2, 2015 8:57 AM, "Wonsuk Lee" notifications@github.com wrote:
The current API is internally consistent. The pattern is basic object vehicle["vehicleSpeed"].get() it's basically the same as vehicle.get("vehicleSpeed") ;).
I don't think removing the object-orientedness helps here. I may still
|
wow, that reply got butchered... fixed |
I have the following ProposalHow about better property and method naming: Why have At first I had the thought of using a
No need for a var vehicle = navigator.vehicle;
vehicle.speed.then(speed => console.log(speed)); But why does it need to be a It's not like it's requesting something that's remote. It should be optimized to read the car speed to be synchronous no? (I could be wrong) Therefore this should be possible: vehicle.speed; // returns the vehicle's speed. Now the only other way to set the Speed of the car would easily be: vehicle.speed = (new speed) // sets the new speed Now if you guys want to be method based: vehicle.getSpeed(); // returns vehicle's speed
vehicle.setSpeed( (new speed) ); // sets the new speed and returns a promise simply because the vehicle will take time to get to the new speed. This is what I meant by better naming I also saw speed histories: vehicle.speedHistory; // returns the same object history that's being used now The vehicle's Doors: vehicle.lockDoor(zone.driver); // locks door Why pass an object Is there anything else to pass to that object perhaps an vehicle.door.set({lock: true, open: true}, zone.driver).then(resolve, reject); Could be turned to: vehicle.lockDoor(zone.driver); // returns a promise
vehicle.openDoor(zone.driver); // returns a promise Like you guys mentioned about people having to remember the strings, same thing with an object which is even more complicated than a simple string Again it's all about good method naming. IMHO It's easier and better. This is what I see with the current api: {
vehicleSpeed: {
get() {}, // returns promise
set() {} // returns promise
},
door: {
get() {}, // returns promise
set() {} // returns promise
}
} Now my proposal would be: {
getSpeed() {} // returns car's speed
setSpeed(newSpeed) {}, // returns a promise with the new speed
lockDoor() {}, // returns a promise
unlockDoor() {}, // returns a promise
openDoor() {}, // returns a promise
closeDoor() {} // returns a promise
} In the above all do what the method name says, and takes a callback. Or it could sort of stay the same and be: {
speed: {
get() {}, // returns car's speed
set(newSpeed) {}, // returns a promise with the new speed
},
door: {
lock() {}, // returns a promise
unlock() {}, // returns a promise
open() {}, // returns a promise
close() {} // returns a promise
}
} ConclusionI understand that you guys are with the Also my proposal get's rid of passing objects that can be taken cared of with Good Method Naming. No need for |
But what about subscribe?
|
I only mentioned certain parts everything else that will be implemented will follow the pattern you guys decide on choosing. Since you guys want the |
I disagree. In the first one, In the second one, When you invoke But
It depends on whether you consider vehicleSpeed a property or an object. I was taking the perspective that it is a property and not an object. Using the vehicle.get() does help with code readability and maintenance. It does not require a null check before you invoke if (vehicle.vehicleSpeed) {
vehicle.vehicleSpeed.get().then(
function(result) {
//do some work
},
function(error) {
//handle error
});
} The following seems like cleaner code to me: vehicle.vehicleSpeed.get("vehicleSpeed").then(
function(result) {
//do some work
},
function(error) {
//handle error, including "attribute does not exist"
}); As an aside, let's remind ourselves that JavaScript isn't a pure object-oriented language it's also functional and dynamic, i.e., it's not Java and shouldn't be treated like Java. I'm starting to wonder if both ideas are not quite right. I don't completely agree that they are the same but, yes, in a sense they are similar. Since promises are just a stop-gap before we have generators, I wonder if it would be a good exercise to consider what the API would look like if we were in an ES6 world and had generators. |
Wait @djensen47 I'm confused, I'm not sure if you replied to anything I said. Was any of that towards what I said or to @tripzero If it was towards him, what do you think about what I mentioned? |
I was responding to @tripzero. I haven't gone through your post yet.
|
On Sat, Jun 20, 2015 at 10:14 PM, Dave Jensen notifications@github.com
This is a distinction without a functional difference. If you don't
Is this a performance argument?
You can initialize properties programitically: This bit of code generates properties that are supported by the backend.
This is not correct. All attributes on vehicle are mandatory. No null
|
Let's say //implement vehicle.get()
var vehicle = (function() {
var v = {};
v.get = function vehicleGet(property) {
if (!v[property]) {
//init is defined elsewhere, it creates the object
v[property] = init(property);
}
return v[property].promise();
};
return v;
})(); Using Could a counter argument be that the promise for |
One way to avoid "magic strings" is to use a constant. vehicle.subscribe(vehicleAttributes.SPEED, options, callback) |
@djensen47 did you read my response I kind of solved that problem. I think. |
Yet another idea. Doesn't really address the autocomplete but might help with "lazy loading." var speed = vehicle.getAttribute(vehicle.SPEED);
speed.get(…);
speed.set(…);
speed.subscribe(…);
speed.unsubscribe(…); As @tripzero mentioned on the phone |
@eorroe I think we decided that your proposal would |
Hmm, there might be a way to lazy load with the current API style. @tripzero, what do you think? var speed, engineSpeed;
var vehicle = {};
Object.defineProperty(vehicle, "speed", {
get: function() {
if (!speed) {
//initialize speed here
}
return speed;
},
set: function(val) {
//do nothing, not allowed
}
}); This also has the advantage of preventing |
@djensen47 what do you mean they'll explode? It'll have to be completely rewritten?? |
"Explode" in terms of quantity of methods on the vehicle object. You're basically putting all the methods from vehicle. into vehicle. |
No I said take the last one:
|
That's very similar to the existing API. The nice thing about the current API is that when you have a similar signature for each attribute the attribute object can inherit it's methods from a super class/object. Methods like |
The reason I put
which could be done like this:
Now no need to use an This method naming is what I'm talking about. The only thing I can think of right now that your talking about inheriting is the I'm not sure what the code looks like but I'm assuming this is what has to be done currently inside those Example
no need for that if we have the proper method naming:
IMHO easier on the developer and the API. What happens when there's more options it grows all into the same function:
Why not split it up easier on the API and developer because of auto completion and not having to remember the passed in object's properties |
@eorroe This issue is more about the abstraction over all of the data points, the common elements that each will share. There are exceptions, which are (or should be) covered in the data spec. I would politely ask you to take a look at the vehicle data spec and file separate Github issues for each suggestion. If you would like to see door lock/unlock instead of set(status) then it is a separate issue. |
@tripzero I think you dropped from the call when we were discussing putting together a document about the pros and cons of the two ideas. Do you think you would have some time to create a pull request with such a document? In markdown I guess? My plate is full until after the Seattle face-to-face. Also, in case you missed it, take a look at #37 (comment) |
Yes. I will try to put together something in markdown probably on this On Wed, Jul 15, 2015 at 11:19 AM, Dave Jensen notifications@github.com
|
Some thoughts on this issue after quickly scanning through it:
|
@djensen47 I think you are correct. Overriding the get property on vehicle should allow for lazy load with the current API. |
@plehegar Mutantionobserver may be exactly what we want with subscribe -so long as it complies with the mutant registration act, I'm okay with it. |
@djensen47 @tripzero I would like to ping you guys for API design options and pros/cons for each of them. It would be great if you share the status of this item. |
I think we have concluded that the current api can work with lazy loading, On Wed, Jul 29, 2015, 4:24 PM Wonsuk Lee notifications@github.com wrote:
|
@tripzero I think it's very important issue for our specs. So I think even you, I and @djensen47 had been agreed, it would be great if we could have feedbacks from the experts in outside at early stage of the specs. :) @paulboyes @QingAn @tguild @djensen47 @aShinjiroUrata @gbrannonaaa What's your view on this? |
Excuse me for jumping in. I'm basically agree with the signature change. Here are a few remarks:
|
@wonsuk73 so do we still need a pro/con document so that we can get outside feedback? |
@tripzero Yes! That's my opinion! What do you think? |
I agree. |
I support we get feedbacks from the experts in outside. |
Agreed. |
I just entered an issue (w3ctag/design-reviews#81) on TAGs spec-reviews repo on GitHub. Please take a look and add comments to the issue if you would like to add more detail or clarification. |
I think we can close this issue when issue #72 is resolved. |
Closing this issue in favor of #81. |
Currently the API works on this signature pattern:
vehicle.dataPoint.method
But
dataPoint
is, in a sense, just data, i.e, meta-data, itself.Since we are dealing with many dozen data points some of which may or may not be implemented, something the following this seems cleaner.
vehicle.method(dataPoint, params…)
Now it doesn't appear as if there are dozens or hundreds of APIs, now we have 3 or 4. It also leaves some flexibility to the implementation. Maybe, it's not a 1-to-1 mapping to the clib implementation but I don't think it should be. This is JavaScript/ECMAScript not C.
vehicle.get(data, options)
vehicle.set(data, value, options)
vehicle.subscribe(data, options, callback)
vehicle.unsubscribe(data, handler)
vehicle.availableFor...(data)
data
is simply a String, the name of the data point like'vehicleSpeed'
options
include information like zone, event periodThis also avoids the issue where
vehicle.vehicleSpeed.get()
could result in aCannot call method 'get' of undefined
if vehicleSpeed is not implemented.The text was updated successfully, but these errors were encountered: