Closed theresalech closed 4 years ago
joeljfischer
commented
4 years ago Does the JavaScript suite need a LockScreen manager? Is this intended for React Native / other mobile JS UIs? If so, which will be supported? If not, what UI is it intended to lock?
I think either the public APIs should be defined in this proposal, or an agreement should be made (and stated in the proposal) that all APIs and code decisions are at the discretion of the project maintainers.
kshala-ford
commented
4 years ago To @joeljfischer comment:
1.1. In addition to the lock screen there is no JavaScript based variant for audio/video streaming (yet). All other manages are valid and useful for the JS suite.
I would expect the author to propose specific APIs for each manager (may be with a skeleton class with all functions) and describe the logic behind the features where references to existing logic in other libraries should be allowed.
With above said I don' think a single proposal can serve this level of detail for all the managers. This proposal should focus on the SdlManager, LifecycleManager and potentially the PermissionManager and SystemCapabilitiyManager. Subsequent proposals should extend the JS suite with new managers that depend on the foundation managers. This will also allow conversations to be more focused on specific topics.
crokita
commented
4 years ago @joeljfischer @kshala-ford
1.1. By "no JavaScript based variant," do you mean there's a lack of libraries to support the types of streaming that would be needed? Maybe that will be something the implementer will have to write on their own.
// Java version
public void uploadFile(@NonNull final SdlFile file, final CompletionListener listener) {
.....
if (listener != null){
listener.onComplete(putFileResponse.getSuccess());
}
}// JavaScript version
async uploadFile (file, listener = null) {
..... // await used here for asynchronous logic, such as sending RPCs
if (listener !== null) { // callback return style
listener.onComplete(putFileResponse.getSuccess());
}
return putFileResponse.getSuccess(); // async/await return style
}The alternative is to just remove the callback parameters in these methods and only allow the use of the async/await style of handling asynchronous logic. I personally would use only the async/await syntax, but it shouldn't be much more complex to allow for the familiar style existing in other libraries. Thoughts? Is there something else that I missed that could alter the way the APIs can be designed? I can add the skeleton methods to the manager classes if needed.
joeljfischer
commented
4 years ago @crokita It's pretty odd that you took the Java-style interface / anonymous class callbacks instead of using the more JavaScript-style (and Obj-C / Swift-style) callbacks where a function is directly passed. Why is that?
Personally, I think that providing two separate ways to get async callbacks will end up being kind of confusing.
crokita
commented
4 years ago @joeljfischer I am more familiar with the Java library code, and not so much with the iOS library. A lot of how the JS suite is written references the Java suite's base files, so that was why.
theresalech
commented
4 years ago The Steering Committee voted to return this proposal for revisions. The author will revise to remove the Lockscreen Mangager, as well as the Video and Audio Streaming Managers. The author will also add public methods for individual managers, so the Steering Committee can review. If any SDLC Members have specific questions about the manager implementation, those should be communicated to the Project Maintainer, so they can be addressed in the revisions as well.
theresalech
commented
4 years ago The author has updated the proposal to incorporate the feedback from the Steering Committee, and the revised proposal is now in review until 2020-01-28.
joeljfischer
commented
4 years ago 
joeygrover
commented
4 years ago Quoting form the contribution guidelines:
- Rather than promoting the direct manipulation of property values, the use of prefixed get and set methods shall be used. For example: setMajor(1) and getMajor().
- The use of ES6-style class "getters" and "setters" shall be avoided. For example: static get major () { return this._major; } and static set major (val) { this._major = val; }. An exception to this rule is made for enumerations (see "Enumerations" section below).
kshala-ford
commented
4 years ago @joeljfischer brings up a good point. Looking at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get getters seem to be a viable solution and much closer to JS coding style. The guidelines are not set to stone yet.
I have a couple questions to the API:
BaseSubManager.getState()? Would an enum make sense here?OnRPCNotificationListener and OnRPCRequestListener objects for the listener params. I think the pattern of accepting a function instead is more common and more convenient.BaseScreenManager. presentSearchableChoiceSet isn't listed in BaseChoiceSetManager. I think overloading presentChoiceSet with a keyboard function should be enough. presentChoiceSet should return a promise and if fulfilled the promise should return the selected choice.cancelID with dismissKeyboard ? Don't know if this parameter is set anywheredeleteRemoteFileWithName can be just delete accepting one or many file names.audioStreamingState and systemContext being exposed by the lifecycle manager.getInternalInterface should be a public API.joeygrover
commented
4 years ago Just a quick note, any changes to the guidelines at this stage will likely cause a very large delay in the JavaScript Suite library release itself, even beyond anything in this proposal. Since these guidelines already exist, albeit not set in stone, all classes and files have been coded to follow them. If the SDLC wishes to make such changes it will need to be understood a delay in the release would be unavoidable.
crokita
commented
4 years ago The JavaScript suite is being developed to closely follow the design patterns and logic in the Java Suite. This allows for faster development and improved understanding of the codebase amongst the PM's team and SDLC partners. Alterations to the design patterns would extend development time and put the SDLC's release schedule at risk.
The public methods listed in this proposal are intended to act as a reference and may change during implementation due to nuances between Java vs JavaScript, observed behavior of the Java Suite's functionality, and/or general learnings.
Refer to 1.
Callbacks can be used in favor of the two listener classes
Refer to 1.
Refer to 1.
Please note that a best effort will be made to follow the Java API spec. However, the methods listed in this proposal should not be considered final, as we may encounter improvements during implementation that allow us to convert some of the asynchronous methods to support Promises, or that some of the methods may be renamed or no longer deemed necessary. In general, the promise syntax will be favored over listener-style callbacks when appropriate. Some of the listener methods seem to be made with the intention of being invoked multiple times for listening to changes in SDL logic and thus the promise API wouldn't be able to be used there.
Refer to 6.
Refer to 1.
Refer to 1.
Refer to 1.
LifecycleManager is missing some properties/methods and will be updated to support the sub-managers as needed.
getInternalInterface() has been removed in favor of using a private instance of LifecycleManager directly
kshala-ford
commented
4 years ago I jumped into the existing code using sed and some regular expressions. I found 388 get occurrences and 327 set occurrences in around 60 files specifically excluding get methods with params. I think it is possible to change all occurrences to use getter and setter with carefully curated expressions and test them in a reasonable time. Changes would be required on the generator though which I have not checked yet.
My point is if there are benefits when changing logic or some pattern I don't want to reject the idea prematurely.
@crokita
The JavaScript suite is being developed to closely follow the design patterns and logic in the Java Suite. This allows for faster development and improved understanding of the codebase amongst the PM's team and SDLC partners.
from the perspective of a developer who works with Java and JavaScript you are right. In practice this will cause issues as language specific paradigms or common patterns are not taken into account. I am with you e.g. for aligning the APIs of managers like the screen manager with sub managers for text and graphic and for soft buttons. I don't agree with you on the design patterns. We should follow language specific patterns instead. I want to note my personal observation that the Java suite branched of heavily from the iOS managers. Generally we should provide a library that allows app developers to be as productive as possible accepting the effort at the SDLC. App developers are the most important asset to the value of the SDL product.
Alterations to the design patterns would extend development time and put the SDLC's release schedule at risk.
Anything that we discuss in this proposal is not in the 2020 SDLC roadmap. There is no risk whatsoever with changes to the API unless the PM want to amend the roadmap and release the javascript suite including this proposal.
Besides these notes. Can you please provide feedback on my original question?
Can you elaborate the possible numbers of BaseSubManager.getState()? Would an enum make sense here?
Sorry. I have to ask you again for feedback on this item.
Sounds good. Thanks for your feedback.
Sorry. I have to ask you again for feedback on this item.
Sorry. I have to ask you again for feedback on this item.
Yes I agree with you. Promises should be preferred and there doesn't seem to be a smarter approach for multiple invokes like keyboard input (though it could be a function callback).
I have to say I am hesitant to accepting a proposal where the API isn't designed. On so many proposals we were very specific to the code and API. Especially the PM have asked to be very clear and detailed. Now I read that the code should not be considered final and may encounter improvements during implementation. This basically eliminates the SDLC to vote and decide on the desired SDL APIs.
Adding promises don't break existing patterns. Also I want to note again that language specific design should be prioritized.
Sorry. I have to ask you again for feedback on this item. Especially because cancelID is never set it's important to understand how useful this function is.
I want to be honest. I don't think the naming was selected carefully. Especially deleteRemoteFileWithName is based on objc function naming. It hsould not be named this way in Java in the first place. Let's not continue carry mistakes from previous adoptions.
JS developers "never" see the RAI request. Looking for information in the response is not intuitive to developers. Exposing the information in a different way like in an own struct should be preferred. At least the function should be named differently because the request was not triggered by the app developer and the developer wouldn't understand what the response is related to.
Would love to see the missing items in a proposal revision.
Thanks for the feedback. I saw there was some movement in the JS repo regarding making accessors private.
joeygrover
commented
4 years ago From my perspective:
Any changes like this still require time and testing. When making sweeping changes testing each individual class and the system as a whole still takes time. Based on the current timeline for the release of the library it is not scoped for such a change and therefore would likely cause a delay. The amount of delay would still need to be determined. There is really no working around this fact.
Any design pattern changes that are requested will have an impact on the existing code base. Otherwise we would have two distinct code base sections with different design patters which is I think we can agree is not acceptable. If this proposal is accepted we will look to get it added to the roadmap. Providing an SDK with no managers at this point is nearly unusable by anyone that does not have extensive SDL knowledge. At the very least providing the entry point of SdlManager to developers is key so that we can keep a consistent API instead of providing two vastly different ones from one release to the next. This is also is a factor when it comes to writing documentation. If there are no managers, brand new documentation will need to be written for every SDL feature the JavaScript library supports. However, if we introduce the managers we can use the existing documentation with new code samples. Both of these are a huge benefits for getting developers into the ecosystem. I would hope that would be universally supported.
1/. No. Number state values should be acceptable. Values will be SETTING_UP = 0x00, READY = 0x30, LIMITED = 0x50, SHUTDOWN = 0x80, ERROR = 0xC0; They are only used internally to the managers and not exposed to developers so they do not need to be type safe by using an enum which will take more space in memory. We exposed an enum like type structure so that the RPC spec could be supported similar to the other platforms, however, the actual JavaScript platform we are using does not have any provided enum types.
2. Builder patterns are great for public classes that the developer is responsible for creating, eg SdlManager. Configs are great for passing specifics to other managers that will get created by the SdlManager, eg FileManager. I think this design pattern makes sense and should continue to be used.
4. This follows the the JavaSuite library API and is not specific to either Java or JavaScript; that is why it is built the way it is. ScreenManager is the exposed API with its submanagers being private; again this follows the JavaSuite implementation and is not platform specific. If this is still a point of contention, I would suggest either taking this to a vote or a new proposal submitted to update the design pattern for all platforms.
5. See point 4. This is how the JavaSuite library works and is not platform specific so the design pattern should stay the same.
6. & 7. Function passing is much better for multiple calls. Promises are inherently for a single, one time action. Passing a function is much better suited for times when things will be called multiple times. The link here is a good resource explains promises. In short: The Promise object represents the eventual completion (or failure) of an asynchronous operation, and its resulting value. With the most important part an asynchronous operation. They certainly have their place in the library, but for more of a listener type role, they don't fit as well as simply passing a function. To that point, I would be open to voting for a revision that removed the specific "listener" classes and took in function/promises instead.
I think the point about methods not being final was that, during the evaluation we will try to use the Promise syntax more than function passing if it makes sense. Therefore, when implementing the actual code, if it's possible to use a Promise instead of passing a function we will do so. I think this is easily enough agreed to and could be a note added to the proposal. I don't believe the authors are saying that the entire API will simply be changed on a whim; again we are trying to mimic what already exists in the other SDL libraries today
8. This is part of an accepted proposal Please see: 0184-cancel-interaction.
9. I disagree. I believe being clear that we are deleting a remote file is important. It also allows the developer to quickly understand where such file is being deleted. Creating a FileManager.delete type method is not descriptive and the developer would have to rely on the documentation. Also, both other SDKs use this naming so changing it for a single platform creates uniqueness. I would suggest either taking this to a vote or a new proposal submitted to update the method signatures for all platforms.
11. This information is provided through the OnHMIStatus notification that a developer can and will need to listen for. However what method would you like to see? Can you provide them for a possible revision?
nickschwab
commented
4 years ago @kshala-ford
I agree with all of @joeygrover's responses.
Additionally, in regard to using ES6-style getters and setters throughout the project, I strongly advise against it. I’ve used many JavaScript libraries and none of them use ES6 style getters and setters for public-facing APIs because it’s awkward to use them for anyone who is versed in multiple programming languages. Using them would create confusion about whether or not you're directly manipulating a class property.
For example, a developer would do this:
const myInstance = new Thing();
// feels like I'm directly manipulating a class property, which is generally frowned upon, and doesn't allow clean chaining of set commands
myInstance.verboseMode = true;
myInstance.appName = 'Hello World';instead of this:
const myInstance = new Thing();
// evident that I'm not directly manipulating a class property and allows clean chaining of commands
myInstance
.setVerboseMode(true)
.setAppName('Hello World');@joeygrover no doubts to your point. Such a change to use JS getter and setter could add unexpected effort and causes risk of a delay. I am saying that if a change that has potential to improve app development should still be reviewed. At the end this is what you're trying to do with the features of this proposal.
@nickschwab I understand your point but for objc it's not true. I also disagree to to your statement to other JS libraries. The programming design of the SDL library is vastly different to other libraries as we don't use object literals where props are native getters and setters. But at the same time the RPCs are full of properties.
Developers would do myInstance = { verboseMode = true, appName = 'Hello World' }
Understand. If that's a private API I'm completely fine with this approach.
Are you suggesting to use a builder for the lifecycle manager and configs for other managers? The develop branch contains an AppConfig class allowing to configure the SDL lifecycle manager class. The configuration pattern is much more common in JS passing an object with all the configs to the actual object (in our case the manager) you want to use. My suggestion would be to use configs for all the managers which would be closer to sdl_ios and closer to how other libraries are being configured.
Thanks for the notice. I do remember that proposal. On sdl_ios there are two methods in the screen manager. presentChoiceSet and presentSearchableChoiceSet. In the proposal I see both methods in the choiceset manager but not in the screen manager.
👍 got it.
& 7. I may not have years of experience with JavaScript but I do know promises very well. my point was that promises are used in so many ways in the JS APIs for every function that potentially is running asynchronous. Many of our manager functions are async but don't have a completion handler. Returning a promise could be possible to match common practice in JS. Whenever completion handlers exist they should be subject to change to functions. I think we agree here to voting for a revision.
8 Thanks for the reference. I overlooked the inline doc that choice set functions return a number ( * @return {Number} ). Due to JS implicit return notes my only feedback would be to describe the cancel ID in the inline doc which is not required to happen in the proposal. No action required here.
9 Sorry may be I was too motivated to shrink the function name... I looked at sdl_ios and it's called deleteRemoteFileWithName:(NSString *)name which should be named deleteRemoteFile(String name) in Java hence the JS function deleteRemoteFile(name) would be preferred. In fact in Swift the method is called delete(fileName:completionHandler:)
10 That's true. The system capability manager takes over most of the RAI response parameters. Therefore the few remaining params could be moved as well.
11 Ah, gotcha. I did read getCurrentHMILevel... ... sdl_ios has an improved behavior to notify the app for each parameter in the OnHMIStatus notification. This has a huge benefit as the app is notified more specifically and each param is compared with the previous status hence the develoepr won't see multiple notifications with the same HMI level (if being interested into HMI levels...)
crokita
commented
4 years ago person.fulName =... instead of person.fullName =.... But yes, it looks nicer. Again, it's debatable that switching styles is better, maybe even arbitrary. Is it worth the time and effort associated with making the change?crokita
commented
4 years ago Ignore the 2. in my previous comment: It doesn't address that specific point
theresalech
commented
4 years ago The Steering Committee voted to keep this proposal in review, to allow for additional discussion on the review issue.
nickschwab
commented
4 years ago @kshala-ford I'm moving the getter/setter discussion to number 13 so we can better manage the discussion.
start() method can perform the basic validation checks prior to starting the LifecycleManager to ensure that the minimum properties of an AppConfig are provided, as well as a ManagerListener being defined.ScreenManager and related sub-managers should utilized/surfaced in a consistent way across libraries to reduce the likelihood of confusing app developers.getInternalInterface() has been removed in favor of utilizing the reference to the LifecycleManager.set and get method name prefixes (current syntax) is a common development practice across multiple programming languages (not to say that there aren't some exceptions), aligns with the Java Suite, and provides developers peace of mind knowing that they're not attempting to directly manipulate an object property. Furthermore, I agree with @joeygrover's comments regarding the risk to the release schedule that such a change would create, and stressing the importance of providing the SDL Manager layers as soon as possible to make SDL app development easier for third-party developers.kshala-ford
commented
4 years ago Below comments is related to topics that are not marked as resolved by @nickschwab
2, I agree with @nickschwab
3, I understand what you're saying. Do you agree to the statement: Any function that performs a async but finite task should return a Promise. Just looking for a generic sentence otherwise callbacks could be used for functions that are not obviously sending an RPC. We can call this 👍 @nickschwab
5, I'm OK with following the other libraries in what functions managers and other classes should expose. I think we can call this resolved as no action required.
6, Understand and I agree. Adding callbacks to a functionID similar to on() would be nice. My point was regarding finite tasks like presenting a choice set where Promises should be used (that's why I mentioned this is related to 3.). However searchable choice sets or present keyboard shuold have a keyboard listener and return a Promise.
7, I believe we are generally on the same page about Promises. 👍
10, Yes, a system capability manager is much appreciated. I think only a handful params remain not being covered elsewhere (version, language, Diagnostic modes). I am considering to address this separately if time allows but for this proposal we can call this resolved 👍
11, I don't think so. Especially the convenient HMILevel listener is needed by almost every app. I recommend using the approach in sdl_ios having listeners for each parameter in OnHMIStatus.
13, I generally disagree to the PM standpoint but before this conversation ignites a religious fight with philosophical statements I am willing to step back and agree to no action required to this item.
nickschwab
commented
4 years ago 2. Resolved via discussion. AppConfig to be passed into SdlManager instead of implementing a "Builder" sub-class. 3. I agree that this should be our general approach to asynchronous logic in the manager layers. @kshala-ford can this be taken on good faith rather than being specifically stated in the proposal? I can't think of a specific example right now, but I'm afraid that making a blanket statement in the proposal could introduce situations where a callback is deemed appropriate during implementation, which would then require 2+ more weeks of evolution discussion in order to pass an amendment. 5. Resolved via discussion without additional required action. 6. Resolved via discussion without additional required action. See item 3 regarding use of Promises vs Callbacks. 7. Resolved via discussion without additional required action. 10. Resolved via discussion without additional required action. 11. Thanks @kshala-ford, I appreciate where you're coming from here. I recognize that there are discrepancies between the the iOS and Java Suite SDKs, but I ask that we continue to use the Java Suite as a baseline for the development of the JavaScript Suite to expedite development in hope of including the manager layers with the v1.0.0 release, and offer improvements such as enhanced OnHMIStatus listeners in a future proposal/release. Can I get your support on this? 13. Resolved via discussion without additional required action.
kshala-ford
commented
4 years ago 3.I agree. We’re on the same page of the preferred coding pattern. That’s all I wanted. No action required. 👍
theresalech
commented
4 years ago A quorum was not present at the 2020-02-04 Steering Committee meeting, so a vote on this proposal could not take place. This proposal will remain in review until our next meeting on 2020-02-11.
theresalech
commented
4 years ago The Steering Committee voted to accept this proposal with the revisions listed below:
start() method can perform the basic validation checks prior to starting the LifecycleManager to ensure the minimum properties of an AppConfig are provided, as well as a ManagerListener being defined.getInternalInterface() in favor of utilizing the reference to the LifecycleManagertheresalech
commented
4 years ago @crokita Please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in the respective repositories for implementation. Thanks!
nickschwab
commented
4 years ago @theresalech PR ready: https://github.com/smartdevicelink/sdl_evolution/pull/953
theresalech
commented
4 years ago PR has been updated to reflect revisions, and implementation issue has been entered: https://github.com/smartdevicelink/sdl_javascript_suite/issues/123
Hello SDL community,
The review of "SDL 0272 - JavaScript Suite SDL Manager" begins now and runs through January 14, 2020. The proposal is available here:
https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0272-sdl-javascript-manager-layer.md
Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:
https://github.com/smartdevicelink/sdl_evolution/issues/901
What goes into a review?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:
More information about the SDL evolution process is available at
https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md
Thank you, Theresa Lech
Program Manager - Livio theresa@livio.io