jviotti
commented
8 years ago Hi Eugene,
The settings issue is one that has been there for quite a lot of time, mainly due to historical reasons, since the SDK was "built" in the CLI.
All the points you make absolutely make a lot of sense. Ideally, the whole settings stuff should live in the CLI, not in the SDK, and settings would be simply passed as an object to the SDK.
In fact, I've started the work to completely decouple the SDK from the settings in this PR long ago: https://github.com/resin-io/resin-sdk/pull/193 but never actually finished due to the decoupling not being an immediate goal at that moment. My suggestion is to check that PR out, and see if you can resolve the conflicts (its from February) and actually get it merged, which should do most of the work for you.
The PR introduces various changes that align with what you've suggested:
- The SDK stops reading settings from the file-system (from
resin-settings-client, etc), and instead gets passed a plain object. - The
baseUrlis explicitly passed to everyresin-requestcall, so that module stops depending on the settings mess. - Decouple
resin-pinefrom the settings as well.
And some other stuff that I don't really remember anymore. You can check the commit messages on that PR for more details.
Does that help?
On Sat, Sep 10, 2016 at 01:18:40PM -0700, Eugene Mirotin wrote:
As you know I've recently started investigating some lower-level modules and improving them to be browser-compatible and adding tests.
Now when I've reached the
resin-pinemodule I've realized the problem is bigger than I assumed. Both the CLI and the SDK are highly modular, but there's a global design issue that prevents the SDK from working in the browser as is. The issue is that multiple low-level modules are actually not universal libraries because they keep knowledge about how things (mostly settings) are stored on the file system.I've built a diagram to illustrate the modules relations: https://ns-pxnflxuoaj.now.sh/resin-sdk%20modules.html. Note that I've excluded many modules that are universal and are not part of the problem.
So in the ideal world, the SDK is supposed to be a universal library. Whatever is specific to the way the CLI works (like storing the apiUrl in the settings file) should only be known to the CLI itself. The SDK should accept all the information it needs as options.
Let's look at an example. https://github.com/resin-io-modules/resin-pine is a nice library that provides all the sane params to the pinejs-client. But it has several flaws that prevent it from being browser-compatible. The main problem as mentioned above is that here https://github.com/resin-io-modules/resin-pine/blob/master/lib/pine.coffee#L57 it reads the
apiUrlfrom the file system. In the browser there's even no analog of that. The URL is not stored in the localStorage. Instead, it's a constant in the code that should be used to instantiate the SDK. The CLI would still use the same settings library to read the URL (on its own) and then again pass it to the SDK.There're a couple more issues of the similar nature:
- here https://github.com/resin-io-modules/resin-pine/blob/master/lib/pine.coffee#L57 the library talks to a different library to check some data, but doesn't actually use it (it throws if the token is missing, but the actual token is not used). It's a small thing, but it's another example of the knowledge shared across the libraries. In my understanding, the library should just pass the options to the
resin-requestand let it complain (BTW some endpoints may not require auth).- here https://github.com/resin-io-modules/resin-pine/blob/master/lib/pine.coffee#L52 the library relies on the
process.envwhich is Node-specific.- in the same line the
RESIN_API_KEYvariable is checked, but it's never used. It's confusing to find where it is actually read and passed to theresin-requestwhich expects it as part of the options.On the contrary, if we check the https://github.com/resin-io-modules/resin-register-device library we'll see a great example of universal code. It requires the user to pass it a PineJS instance and thus is completely agnostic of the way that instance is created and configured.
Given these examples here's how I see the plan to make the SDK universal:
resin-tokenwill remain as it is (now that it can run in both node and the browser), and will be the only low-level library directly dealing with storage (because we want it to persist the token without our intervention).- as far as I can see
resin-settings-clientis currently serving two purposes: it keeps some useful defaults and shared info, and it also reads the user overrides from the file system. It has to be split somehow so that the first part can be used in the browser (if ever applicable).resin-pine,resin-requestwill be made independent fromresin-settings-clientandprocess.env. Whatever they need should be passed to them as options. These are the modules I've found, but there can be more. This is a breaking change and will require the major versions to be released.resin-sdkis made agnostic as well. It should receive the entire set of options, and pass them to sub-modules accordingly. Again it will require a new major version.- Finally,
resin-clishould be responsible for reading he options (usingresin-settings-client) and passing them to the SDK.Then we'll be able to make the UI do the same but use its own means to get the properties (for example, the api key there will always be undefined, and the api URL is part of the UI code, etc.).
You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: https://github.com/resin-io/resin-sdk/issues/221
Juan Cruz Viotti Software Engineer
emirotin
craigmulligan
As you know I've recently started investigating some lower-level modules and improving them to be browser-compatible and adding tests.
Now when I've reached the
resin-pinemodule I've realized the problem is bigger than I assumed. Both the CLI and the SDK are highly modular, but there's a global design issue that prevents the SDK from working in the browser as is. The issue is that multiple low-level modules are actually not universal libraries because they keep knowledge about how things (mostly settings) are stored on the file system.I've built a diagram to illustrate the modules relations: https://ns-pxnflxuoaj.now.sh/resin-sdk%20modules.html. Note that I've excluded many modules that are universal and are not part of the problem.
So in the ideal world, the SDK is supposed to be a universal library. Whatever is specific to the way the CLI works (like storing the apiUrl in the settings file) should only be known to the CLI itself. The SDK should accept all the information it needs as options.
Let's look at an example. https://github.com/resin-io-modules/resin-pine is a nice library that provides all the sane params to the pinejs-client. But it has several flaws that prevent it from being browser-compatible. The main problem as mentioned above is that here https://github.com/resin-io-modules/resin-pine/blob/master/lib/pine.coffee#L57 it reads the
apiUrlfrom the file system. In the browser there's even no analog of that. The URL is not stored in the localStorage. Instead, it's a constant in the code that should be used to instantiate the SDK. The CLI would still use the same settings library to read the URL (on its own) and then again pass it to the SDK.There're a couple more issues of the similar nature:
resin-requestand let it complain (BTW some endpoints may not require auth).process.envwhich is Node-specific.RESIN_API_KEYvariable is checked, but it's never used. It's confusing to find where it is actually read and passed to theresin-requestwhich expects it as part of the options.On the contrary, if we check the https://github.com/resin-io-modules/resin-register-device library we'll see a great example of universal code. It requires the user to pass it a PineJS instance and thus is completely agnostic of the way that instance is created and configured.
Given these examples here's how I see the plan to make the SDK universal:
resin-tokenwill remain as it is (now that it can run in both node and the browser), and will be the only low-level library directly dealing with storage (because we want it to persist the token without our intervention).resin-settings-clientis currently serving two purposes: it keeps some useful defaults and shared info, and it also reads the user overrides from the file system. It has to be split somehow so that the first part can be used in the browser (if ever applicable).resin-pine,resin-requestwill be made independent fromresin-settings-clientandprocess.env. Whatever they need should be passed to them as options. These are the modules I've found, but there can be more. This is a breaking change and will require the major versions to be released.resin-sdkis made agnostic as well. It should receive the entire set of options, and pass them to sub-modules accordingly. Again it will require a new major version.resin-clishould be responsible for reading he options (usingresin-settings-client) and passing them to the SDK.Then we'll be able to make the UI do the same but use its own means to get the properties (for example, the api key there will always be undefined, and the api URL is part of the UI code, etc.).