Skip to content
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

Closed
theresalech opened this issue May 22, 2019 · 20 comments
Closed

[Accepted with Revisions] SDL 0235 - SDL JavaScript library #742

theresalech opened this issue May 22, 2019 · 20 comments

Comments

@theresalech
Copy link
Contributor

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:

  • Is the problem being addressed significant enough to warrant a change to SDL?
  • Does this proposal fit well with the feel and direction of SDL?
  • If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    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]

@joeljfischer
Copy link
Contributor

| 1.

As this specification is XML structured it makes sense to develop a code generator to export all the RPCs, structs and enums to usable code.

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.

The first release should come with a base/abstract definition of the transport layer. Based on this definition a TCP socket client transport and a WebSocket Server transport described in Cloud App Transport Adapter should be implemented with this proposal.

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.

The downside of having another SDL library is the increase of SDL maintenance. The project maintainers may need to assign individuals who are responsible for managing issues, projects and releases, and reviewing pull requests.

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.

@ghost
Copy link

ghost commented May 23, 2019

|1.

👍 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.

|2.

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.
b) The proposal is suggesting a TCP client transport... This transport is not related to Cloud Transport Adapter. This transport should allow connections over WiFi and is very useful for development and testing purposes. The proposal doesn't mention use cases for this transport because the proposal should not limit the use of this transport. I don't want a TCP transport being marked as the "debug"/"develop" transport. This transport should be implemented with node.js Net framework. Looking at the requirements

@joeljfischer
Copy link
Contributor

|2. b.

This transport should allow connections over WiFi and is very useful for development and testing purposes. The proposal doesn't mention use cases for this transport because the proposal should not limit the use of this transport. I don't want a TCP transport being marked as the "debug"/"develop" transport. This transport should be implemented with node.js Net framework.

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.

@ghost
Copy link

ghost commented May 24, 2019

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.

@joeygrover
Copy link
Member

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 CustomTransport as was necessary for the Java EE library to work with Java Beans without adding proprietary code into the library. This would actually allow for developers/OEMs to create a TCP transport if it works outside the normal use cases. Giving the developer the ability to use custom transport implementations protects against future needs by OEMs which could include various IPC methods for embedded devices.

@joeygrover
Copy link
Member

Also, it mentions the project maintainer will be responsible for maintenance, however, is there a plan for who would be performing the initial creation?

@ghost
Copy link

ghost commented May 28, 2019

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...

@joeljfischer
Copy link
Contributor

joeljfischer commented May 28, 2019

I'm not sure that we have enough definition around this proposal.

  1. The TCP transport is not currently a primary transport. This proposal seems to be making it a primary transport without stating it as such, which I'm not entirely comfortable with agreeing to, and I'm not sure that Core is prepared to handle as a production, primary transport. Saying that it's easy is true as a debug transport, but I'm not sure that the implications of making it a primary transport have been fully thought through or detailed. I don't know how that would possibly work in a production server (cloud) environment, and I don't think Core is set up to handle it as an intra-vehicle (embedded) environment (e.g. to pass out ip / ports to embedded apps either on-demand, or via a policy table).

    Furthermore, I think Web Sockets can better handle production needs in both cloud and embedded environments. Some changes may be needed to better support debug applications (e.g. by entering in a socket address on the HMI emulators), or TCP could be added as a debug-only transport. I simply haven't seen the author provide a use-case that requires TCP over the already accepted Web Socket transport.

  2. As far as using NPM modules for sockets (and potentially TCP), I'm okay with it, but I think we need to understand that we're creating some lock-in to the Node platform (which is probably okay since they're the giant in the JavaScript server space). However, I think the repo should probably be renamed to sdl_node to reflect this.

  3. I think we're in agreement that having the transport be abstract enough to allow new OEM (or app developer) created transports is a need. I believe @joeygrover's example of an intra-vehicle based communication transport for embedded applications is a good one.

  4. I think this proposal needs to add the creation of an example app to its list of tasks.

@ghost
Copy link

ghost commented May 28, 2019

@joeljfischer

|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.

@joeljfischer
Copy link
Contributor

|2. The reason we moved to java_suite was because the platforms that could be used expanded. We're not about to change sdl_ios to sdl_objc, for example. If the library can only be used on a node.js server, then it should be sdl_node.

|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?

@ghost
Copy link

ghost commented May 28, 2019

|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

@joeljfischer
Copy link
Contributor

|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. sdl_ios, and even sdl_java_suite is a suite not just java because it's composed of java_se, java_ee and android platform releases), as well as it helps with developer expectations. This isn't a JavaScript client library, for example, it's a node-based library (or even NPM module) for Node servers.

|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.

@nickschwab
Copy link
Contributor

nickschwab commented May 29, 2019

@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 sdl_javascript_node and it would be important to note that embedded apps built upon the library would be required to be able to run on the head unit as a Node.js server with a port open to incoming connections from Core. The non-transport-specific pieces of the library should be abstracted into a plain JavaScript component/repository called sdl_javascript and used as a dependency in sdl_javascript_node. All this said, it seems fitting to remove TypeScript as an implementation detail, in favor of plain JavaScript (ES2015) for sdl_javascript and Node.js 10.3.0+ for sdl_javascript_node. These version targets should be strictly defined in the proposal.

|3. In my opinion, the sdl_javascript_node library should only implement SDLC-approved cloud transports (WebSockets) in the initial version, but be developed in a way to allow the quick addition of new transports in the future.

|4. I agree - a sample app (HelloSDL or otherwise) using the sdl_javascript_node library should be built as part of this proposal. However, I think it should be maintained in a separate repository such as sdl_javascript_node_examples to keep the SDK libraries as lean and production-ready as possible.

@ghost
Copy link

ghost commented Jun 4, 2019

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.

  1. Not only can it be executed on browsers but also in Node.js.
  2. TS will keep the code easier to maintain over the next years reducing maintenance burden.
  3. TS can be transformed (transpiled) to plain JavaScript code when creating releases.
  4. TS allows intellisense and lint. Source map files can be created out of it making it easier for app developers to understand our API.

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:

  • root the repository root folder which should include README.md etc.
    • base including all source files that are the base for any JS runtime
      • src/com
        • livio/BSON this folder should include the JS implementation of the BSON spec
        • smartdevicelink
          • proxy source related to the proxy and RPC spec implementation
          • protocol source related to the protocol spec
          • session session related part of the protocol spec
          • transport base source files that transports can inherit from
    • node.js all Node.js related source files etc.
      • build building scripts for npms
      • src/com/smartdevicelink/transport contains all transport implementations for Node.js

|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.

@joeygrover
Copy link
Member

  1. I'm in support of a naming close to sdl_javascript_suite to align with the existing repos and seeing the intention of supporting multiple platforms using the same base source set of javascript/typescript. Each module can be published individually, but the repo itself can contain all the related modules.

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.

@nickschwab
Copy link
Contributor

@kshala-ford @joeygrover

Repository structure: Given @joeygrover's attestation that the _suite approach is working well for the Java libraries, I'm happy to get behind a single sdl_javascript_suite repository with the contained components individually published as modules. I advise not including a specific directory structure in this proposal, as this is a detail which should get worked out during architectural design/implementation.

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.

@ghost
Copy link

ghost commented Jun 4, 2019

I think we are in an agreement. Just trying to recap everything:

  1. Create a single repo with base source code and Node.js support (transports and build scripts for npm creation).
  2. PM to lead project management. Still, I see benefits involving donators for better alignment so that we can work together on the architecture and make donations easier and faster. When breaking up pieces that can be donated it's helpful knowing the details right away.

I believe there are two open questions left:

  1. TypeScript or JavaScript? I understand your point of missing knowledge but I want to note again the benefits of typed code. It helps app developers in writing apps but also us in maintaining the repo for the next years as the codebase will grow.
  2. ECMA version? 2015 or 2017?
    a. 2017 allows code reduction using async/await and other more modern features. It also allows app developers the same giving them the chance to reuse more existing code. The downside is that older Node.js versions don't support 2017 (so do older browsers or other JS platforms).
    b. 2015 allows our new library to be functional on pretty much any kind of JS platform. However, this might make SDL integrators to use a Node version without 2017 support hence apps cannot use modern APIs.

I guess the above items should be decided by the SC.

@theresalech theresalech changed the title [In Review] SDL 0235 - SDL JavaScript library [Accepted with Revisions] SDL 0235 - SDL JavaScript library Jun 5, 2019
@theresalech
Copy link
Contributor Author

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.

@theresalech
Copy link
Contributor Author

theresalech commented Jun 5, 2019

@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!

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Jun 5, 2019
@theresalech
Copy link
Contributor Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants