-
Notifications
You must be signed in to change notification settings - Fork 121
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
[Accepted with Revisions] SDL 0235 - SDL JavaScript library #742
Comments
| 1.
Note that I have in fact presented a proposal for this exact feature for Java / iOS. If that proposal were accepted, I'm sure it would largely be reused for this library. | 2.
I'd be a bit concerned about defining that that these transports must be created without defining how they will be created without the use of 3rd party libraries. Additionally, the Cloud App Transport proposal only defines WebSockets as an acceptable mode of transport. This proposal seems to be smuggling in a TCP transport as well. Also, I was unable to find a pure javascript API for TCP, whereas there is one for WebSockets. I would therefore recommend that the TCP requirement be removed from this proposal. |3.
I think this is the primary issue with this proposal. The question of development and maintenance is important for a fixed-budget consortium. If this is important to OEMs, it can certainly be done, but all projects may need to reduce the number of features per year that can be implemented due to the increased cost of implementing every feature (because it must be implemented on one additional platform) and the increased cost of general maintenance on a new proxy library. I am certainly no authority on the money issues of the SDLC, however, so I'll defer further conversation on this point to others. |
👍 I guess we had the same idea at the same time. I do support your proposal and would see the RPC generation being used for JavaScript.
I want to note that some specifics were removed as per maintainers suggestion as they are seen as development details. I guess there are a couple of issues scrambled together. First of all, the sentence should have started with web sockets (for cloud apps) then tcp (general purpose). a) The proposal is suggesting a WebSocket-Server transport according to Cloud Transport Adapter. This should enable cloud apps developed in JavaScript. There are a couple of libraries supporting WebSockets, I guess I would recommend the best compatibility to boost::beast. |
|2. b.
It looks like you got cut off at the end there, so I'm not sure if you had more comments to make. First, I don't believe that TCP needs to exist as a transport for debug purposes. The websocket transport can do just fine for both debug and production purposes and is how we already do it with the Java cloud library and Core. Second, I'm against including this transport not only because it seems to me to be unnecessary, but also because it requires the Node.js framework. This library is no longer the "javascript" library, but the "node.js" library. With that said, the websocket server transport also seems like it may require a 3rd-party library such as a Node package. Therefore, I would advocate for the transport to easily allow plugins, but that this library be as "pure-javascript" as possible and not require that the developer use one server framework or another for its functionality. If a pure javascript websocket is possible, we should use that. If it requires node, then we need to decide which transport plugins to provide to support as many developers as possible. Node.js is certainly extremely popular, but I don't want to lock out any developers if we can help it. |
Yes you are right... something got cut off after some network issues... I think my comment contains all the important information still. I believe not everything you wrote is correct or true. I do believe TCP should be provided and I don't believe WebSockets can do "just fine". The principles between the both protocols are heavily different. Why? Core acts as a TCP server but it also acts as a WebSocket client. The reason for being a client makes total sense for cloud apps and the transport adapter. However, it requires one app (the OEM app store) to connect without the adapter so that it can inform Core about the cloud app. Unless the HMI has developer tools you will need another "regular" app doing this. Therefore I see TCP very useful as a transport. I think I have to explain why there is no such "pure javascript". I assume you mean a browser like Chrome with pure javascript? Chrome is one runtime environment for JS, just as Firefox or any other browser... and there is node.js. Each browser is a different runtime but they use to provide mostly the same APIs like the browser object model or document object model. node.js is also a runtime but it comes with lots of different APIs. I guess the "pure javascript" is the subset of common APIs between the runtimes we choose to support. Pure could also mean the basic APIs specified in ECMAscript (though I cannot confirm that until I finished reading the 801 pages of the spec). I understand what you mean with plugins but I would instead suggest the approach of the java_suite repository. a base folder should contain all the code that can be used throughout all runtimes. This should at least include the proxy and the RPCs as well as the protocol layer and the abstract definition and code of the transport layer. Each runtime we support may include extra transports or other code required which goes into a separate folder. This may include scripts to export and deploy the library to the code repository of the runtime (for node.js it would be npm). Depending on how far the runtime supports dependencies and plugins we can provide each transport as separate packages. The TCP transport should be implemented with this proposal using node.js. For a WebSocket server I also recomment node.js using socket.io as the 3rd party library. |
I believe the use case mentioned (app store before endpoints updated) is already covered. By adding the end point to the preloaded policy table the HMI should recognize that there is an app store to connect with and display the icon on the HMI. Then per the proposal, when the user selects the icon it will connect out through the standard websocket paradigm. I think an alternative would be to implement a |
Also, it mentions the project maintainer will be responsible for maintenance, however, is there a plan for who would be performing the initial creation? |
Thanks for the note @joeygrover. I wasn't aware o the preloaded policy to include endpoints (and potentially this to be the OEM store). Regarding the custom transport I believe there could be interesting use cases solved with this (though the limited common API of JavaScript can make it difficult to work with generic streams...). However, I still think a TCP transport should be included in the library as the implementation is very lightweight and easy. I guess (because it'll be for node.js) the TCP transport could be offered as a separate package in npm. Same could be done for the WebSocket transport. The Ford Motor Company is looking into this library in the near future and would provide resources to implement the components of this and future proposals but also to test the code and fix bugs. I cannot promise when and how this is going to happen from my pay level though... |
I'm not sure that we have enough definition around this proposal.
|
|1. I'm afraid the assumption here is wrong. The proposal is not trying to make TCP a primary transport or anything. It's just trying to provide all transports that can be implemented. TCP transport is a transport in the other libraries and I don't see any reason to not include it in this new library. Today for Ford this transport is at least needed for development purposes with emulators or test devices at. I don't understand why this transport must not be part of the new library. It'll not challenge any production connectivity (like websockets). |2 I cannot agree to this. We just moved from android (which is a platform running java) to java_suite (support pretty much any platform). The repo should not be named by an environment but to the language it's supposed to be used for. |3 Are you talking about the CustomTransport? I'm not sure if that's the case. Sure we need a GenericTransport definition but I don't think I have enough input to CustomTransport yet to make a decision. |4 agree. A node.js variant of HelloSDL can be added as part of the proposal. |
|2. The reason we moved to |3. A Generic transport abstract class or interface that can have individual transports plugged into it would by definition permit OEMs to build custom transports, right? |
|2 I think we see it from different perspectives. I want to avoid being unnecessary specific. I don't see a reason for being specific. |3 This is true. However a CustomTransport was created in Java for a purpose which is not necesary in the JavaScript environment. A subclass of BaseTransport is always possible. I believe this is a bad idea for each OEM to create their own TCP transports |
|2. Yeah I agree we're seeing it from different perspectives. My perspective is that it's consistent with the rest of the project (e.g. |3. I definitely don't want every OEM to create their own transport implementation. I'm okay with a TCP transport being created based on your notes above, but I do see a potential need for OEMs to create transports for embedded transports, etc., and I think this item needs to be baked into the library from the start. |
@kshala-ford @joeljfischer |1/2. Plain JavaScript is unable to act as a client or server for TCP connections. Similarly, plain JavaScript is unable to act as a WebSocket server, but it can act as a WebSocket client. Node.js can act as a server and client for both transports. For supporting cloud-based apps (which I believe is the primary goal of the JavaScript SDK), I don't see any problem with using Node.js-specific dependencies since it is the most popular server-side JavaScript runtime, is based on the widely-adopted V8 JavaScript engine, and is essentially required to align with the Cloud Transport proposal. The Node.js-specific library should be called |3. In my opinion, the |4. I agree - a sample app (HelloSDL or otherwise) using the |
My idea of this new repository is to be similar as the Java suite. Looking at the MDN JavaScript Reference that list pretty much everything that is available on any JS runtime (browsers and Node.js) I am absolutely sure that the RPC spec with the proxy as well as the protocol spec can be implemented based on this reference. As you already know from the PM workshops we are not only interested into supporting Node.js but also browsers. This proposal and supporting cloud-based apps is not the primary goal but the first milestone to a broader usage. I don't agree to remove TypeScript.
The ECMAScript Version we should develop with should be ES2017 as this version comes with Promises and async/await. See https://node.green/#ES2017. I suggest to create the repository named "sdl_js_suite" with the folder structure as followed:
|3 The TCP server implementation in SDL Core was available long before the SDLC. I never suggested TCP to be used for cloud apps (and the transport adapter). I am primarily proposing the TCP client transport being added to JS because this transport is already available. This transport is part of all SDL libraries. Not adding it would be a new SDLC decision against existing technologies. Not adding a TCP transport to node makes the JS library unavailable to e.g. the Ford SYNC3 Emulator but also to SYNC systems with WiFi. Without this transport developers can not test their app on Ford systems before the app gets submitted for a cloud app. I don't see the reason to not add this transport which was always available. The TCP transport should be implemented with the library for debugging purposes. It is not intended to be used in a production environment within this proposal. |
However, I would caution against a shotgun style approach when supporting javascript platforms. Each and every addition has to be carefully assessed for its value as the SDLC will be on the hook for not only creating but maintaining each of these platforms. Each addition has the potential to add a mountain of technical debt. So with that in mind, this proposal is for creating the base source set and a node module based on that source set? Or simply the base source set? As far as TypeScript for JavaScript, I'm torn. I prefer more structured languages as they can help reduce learning curves, but I do see the appeal of sticking to JavaScript for the base source set so that all modules can import it without having to do any extra work via transpiling. So for this I would have to look towards the expert on my team and say JavaScript is the better option as project maintainer is likely the ones having to maintain this library and all supersets of JavaScript can use JavaScript. Going back to a question I had earlier regarding the creation of this project, I think there needs to be a heavily coordinated effort between the donations of to this proposal and the maintainers. Creating the entire project and throwing it over the wall for other engineers to maintain is an extremely bad idea. For the sake of the project maintain code styles and overall maintainability I would rather see the project maintainer take on the creation. Since I'm sure there is a deadline for this project that might not be feasible. So instead I would suggest that the project maintainer be the lead architect and project manager for the proposal. This would enable the PM to break up the pieces that can be donated, review smaller chunks of donations instead of one large project, and ensure that the quality and maintainability of the library is of a high level. |
@kshala-ford @joeygrover Repository structure: Given @joeygrover's attestation that the TypeScript: As primarily a JavaScript/Node.js developer, I’ve personally never had an issue with many of the “problems” TypeScript claims to address, but I can see where folks who are more accustomed to strict languages would be more comfortable with it. That said, not all JavaScript developers are fluent in TypeScript, which should be considered a major disadvantage to ongoing maintenance and I recommend acknowledging this in the proposal. Library maintenance: I strongly agree with @joeygrover on this topic. The amount of ongoing maintenance the creation of this JavaScript suite will generate isn't trivial. For this reason it is vital for the PM to be highly involved in the creation of the library. If the SDLC's expected delivery timeline allows, I would prefer to see this initially developed entirely by the PM. |
I think we are in an agreement. Just trying to recap everything:
I believe there are two open questions left:
I guess the above items should be decided by the SC. |
The Steering Committee voted to accept this proposal with revisions. The revisions will include the first two items in this comment from the author. Regarding the open questions, the Steering Committee voted to use JavaScript and ECMA 2017. |
@kshala-ford 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 create an issue for implementation. Thanks! |
Proposal has been updated to reflect agreed upon revisions and repository has been created, with details from proposal included in repo README: https://github.com/smartdevicelink/sdl_javascript_suite |
Hello SDL community,
The review of "SDL 0235 - SDL JavaScript library" begins now and runs through May 28, 2019. The proposal is available here:
https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0235-sdl-js-library.md
Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:
#742
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:
Please state explicitly whether you believe that the proposal should be accepted into SDL.
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
[email protected]
The text was updated successfully, but these errors were encountered: