Modular architecture with appmodules #7

mrodem · 2016-09-30T13:31:58Z

This PR introduces a major architectural change. Instead of having only one mode of operation (BLE), we now support multiple "appmodules". When starting the application, the users can now choose which appmodule they want to use. See doc/README.md for more information on appmodules.

The appmodule approach has been implemented by having multiple npm packages in the same repository (monorepo). To handle multiple packages, we are using lerna which iterates over packages in the packages directory, performing operations like install, build, test, publish, etc.

The scripts in the top-level package.json has been changed to use lerna, so we can still run npm install to initialize the project and npm start to start the application. The difference is that these commands now operate on packages in the packages directory. When running npm install, all dependencies for all packages will be pulled down, and local packages will be linked and built. Common development dependencies are defined in the top-level package.json, so that they can be reused by all packages.

We currently have the following packages:

packages
├── nrfconnect-appmodule-ble
├── nrfconnect-appmodule-sample
├── nrfconnect-core
└── nrfconnect-loader

The BLE-code that was previously on the top-level of the project has now been moved to nrfconnect-appmodule-ble. A sample appmodule has been added to nrfconnect-appmodule-sample, to make it easy to start developing new appmodules. The new entrypoint of the application is nrfconnect-loader, which presents the user with an appmodule selection UI. Common code and resources are located in nrfconnect-core.

Unit testing with jest has been introduced, and a few simple tests have been added for the nrfconnect-loader and nrfconnect-appmodule-sample packages. Tests are executed by running npm test, either in the root directory (tests all packages), or in each package directory.

React et.al is defined as peerDependencies in nrfconnect-core/package.json. These are installed at development time, but they are not bundled with the library, so those who depend on nrfconnect-core need to add their own React dependency in their package.json. This works fine when the library is built and published, but during development there will be a node_modules directory (with React) in nrfconnect-core, and this causes problems. When other projects import React components from nrfconnect-core, the Node module loader will first look for React in nrfconnect-core/node_modules. The other projects will provide their own React dependency, and that leads to two copies of React being loaded at the same time. In general, we do not want anything from nrfconnect-core/node_modules to be loaded by other projects. To avoid this we patch Node's module loader. For more info see: https://www.sharpoblunto.com/News/2016/01/25/testing-react-component-libraries-using-npm-link

mrodem · 2016-10-04T07:33:01Z

Stumbled upon this: facebook/create-react-app#675. The create-react-app project is planning to use a webpack alias to tackle the duplicate react issue (0c9b61f). I tried the proposed webpack solution last week, but could not make it work. Will be interesting to see create-react-app's solution.

CoqRogue · 2016-10-05T10:52:21Z

packages/nrfconnect-appmodule-ble/js/containers/DiscoveredDevices.js

@@ -87,7 +88,7 @@ class DiscoveredDevices extends React.PureComponent {
                <div>
                    <h4>
                        Discovered devices
-                        <img className='spinner' src='resources/ajax-loader.gif' height='16' width='16' style={progressStyle} />
+                        <img className='spinner' src={spinnerImage} height='16' width='16' style={progressStyle} />


Should spinner be made a separate component with size as a prop? This is at least the third time I see it in this commit.

Agreed. Spinner should be a component. Will add it.

CoqRogue · 2016-10-05T10:52:57Z

packages/nrfconnect-appmodule-ble/js/index.js

@@ -10,8 +10,6 @@
 *
 */



Why not strict any more?

According to the ES2015 spec, module code is always strict mode. Should not be necessary to add "use strict" anymore for ES2015 modules.

http://stackoverflow.com/questions/31685262/not-recommended-to-write-out-use-strict-with-es6

CoqRogue · 2016-10-05T10:58:24Z

packages/nrfconnect-appmodule-sample/js/actions/appActions.js

@@ -10,10 +10,10 @@
 *
 */

- 'use strict';


Why remove strict?

CoqRogue · 2016-10-05T11:04:08Z

packages/nrfconnect-core/index.js

+        minHeight: 499,
+        show: false
+    }, options);
+    var browserWindow = new electron.BrowserWindow(mergedOptions);


Should use let instead of var (also valid for rest of file).

Sure. Used var here to be consistent with what has been done before. I guess it was done this way because this code is not being transpiled, but let is supported now, so I agree that let is the way to go.

Will switch to let for all the main index.js files.

CoqRogue · 2016-10-05T11:05:14Z

packages/nrfconnect-core/js/components/input/SelectList.jsx

+import React, { PropTypes } from 'react';
+import { FormGroup, ControlLabel, FormControl, InputGroup } from 'react-bootstrap';
+
+class TextInput extends React.PureComponent {


Should be named the same as the file.

Nice catch. Will fix.

mrodem · 2016-10-05T12:40:26Z

Added Spinner component, changed from var to let, and now using correct name in the SelectList component. Good feedback, thanks.

bihanssen · 2016-10-05T18:25:51Z

Great work, thanks!

mrodem added 13 commits September 23, 2016 08:56

Get rid of error message when exiting app in dev mode

f85aa02

Enable sourcemaps for debugging in Chrome devtools

4e5f329

Introduce monorepo with core, loader, and BLE appmodule

2f9ac95

Add testing with jest for the new appmodule loader components

cd8e3ab

Ability to run test/clean/build for all packages from the root dir

8044f62

Replace settings.json with NODE_ENV variable

16e4a9a

Bundle hex files with webpack instead of copying them in setup

17dc4a0

Use new npm commands to clean/build project in setup

7899e39

Create a minimal sample appmodule

097a91e

Skip loader if there is only one appmodule

5b5ffb3

Add info about testing and appmodules in README

7c3fd48

Document appmodule configuration and development

f8a8d8b

Bump to latest lerna version for improved Windows support

c06318a

CoqRogue reviewed Oct 5, 2016

View reviewed changes

mrodem added 3 commits October 5, 2016 14:26

Add spinner component

c93fd73

Change from var to let in index.js files

bd1b13f

Use correct component name in SelectList

5aeef44

CoqRogue approved these changes Oct 5, 2016

View reviewed changes

bihanssen merged commit 019f595 into master Oct 5, 2016

bihanssen deleted the plugin-loader branch October 5, 2016 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modular architecture with appmodules #7

Modular architecture with appmodules #7

mrodem commented Sep 30, 2016 •

edited

Loading

mrodem commented Oct 4, 2016 •

edited

Loading

CoqRogue Oct 5, 2016

mrodem Oct 5, 2016

CoqRogue Oct 5, 2016

mrodem Oct 5, 2016

CoqRogue Oct 5, 2016

CoqRogue Oct 5, 2016

mrodem Oct 5, 2016

CoqRogue Oct 5, 2016

mrodem Oct 5, 2016

mrodem commented Oct 5, 2016

bihanssen commented Oct 5, 2016

Modular architecture with appmodules #7

Modular architecture with appmodules #7

Conversation

mrodem commented Sep 30, 2016 • edited Loading

mrodem commented Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrodem commented Oct 5, 2016

bihanssen commented Oct 5, 2016

mrodem commented Sep 30, 2016 •

edited

Loading

mrodem commented Oct 4, 2016 •

edited

Loading