electrode-io / electrode-native

A platform to ease integration&delivery of React Native apps in existing mobile applications
https://native.electrode.io
Other
727 stars 113 forks source link

Distinguish between MiniApp / JS API impl #306

Closed belemaire closed 6 years ago

belemaire commented 7 years ago

JS API implementations are pretty much similar to MiniApps in the sense that they contain only JS code, are published to NPM, versioned and can be updated through OTA.

However we don't have any specific distinction in the Cauldron between the two, neither in terms of commands.

We should do the necessary to differentiate between the two structurally, even if in term of platform logic, most of the same logic as for MiniApps will apply.

Depends of https://github.com/electrode-io/electrode-native/issues/307 to be achived

deepueg commented 6 years ago

The current implementation allows an api implementation(js and native) to be added to cauldron as dependencies. When a container is generated it looks for js and native implementations and generates the code accordingly.

https://github.com/electrode-io/electrode-native/pull/372

belemaire commented 6 years ago

Don't think it's a good idea to store these dependencies in the nativeDeps array of the Cauldron.

First, it defies the naming (nativeDeps => native dependencies). A JS API implementation is pure JS code, so it is not a native dependency at all.

We could just say, well let's renaming nativeDeps into dependencies and be done with it, but in that case why do we store only native dependencies and JS API implementations there, and not all JS dependencies part of the MiniApp as well. Difficult to justify.

Also mixing native dependencies and JS API impl in this object is probably not a good idea as it will result in quite a few additional conditionals in the code to check each dependency and infer if it's a native one or if its a JS API implementation. It's easier to keep this array as nativeDeps and know for sure that it only include native dependencies (a lot of versioning checks around container generation / CodePush or even adding things to Cauldron are done by just looking at the native dependencies).

And what happens when it comes to CodePush ? For each CodePush update, we create an entry in the Cauldron listing all the pure JS 'components' (MiniApps only for now) that have been deployed part of this update. Because JS API implementations are pure JS standalone project, new versions can also be released through CodePush, and should also be stored in this entry (for ex https://gecgithub01.walmart.com/react-native/walmart-cauldron/blob/master/cauldron.json#L1918)

This old storage format for CodePush entries has been updated (in the dev version), new CodePush entries are formatted as seen in my dev Cauldron (https://github.com/belemaire/dev-cauldron/blob/master/cauldron.json#L72). An object for each entry, containing a miniapps object listing the miniapps part of this update. Adding a jsApiImpls object makes sense (whereas in previous CodePush entries storage, it was under miniapps so no way to distinguish).

This issue therefore still makes sense. JS API implementation are very similar in term of platform logic to MiniApps. They are less similar with native dependencies, so it does not make sense to store them in the native dependencies array in the Cauldron.

We should add new arrays in the Cauldron to specifically hold JS API implementations. In addition to miniapps and nativedeps we should have a jsApiImpls array for Container and for CodePush entries.

Looking at the new Cauldron format, refactoring CodePush entries, https://github.com/belemaire/dev-cauldron/blob/master/cauldron.json#L72, it appears that the schema used for the Cauldron is not really proper (better than the old, but still not there, especially if we distinguish jsApiImpls) :

"nativeDeps": [
  "react-native@0.50.1",
  "react-native-code-push@5.1.3-beta"
],
"codePush": {
  "Staging": [
    {
      "metadata": {
      },
      "miniapps": [
        "code-push-test-miniapp@0.0.19"
      ]
    }
  ]
},
"miniApps": {
  "container": [
    "code-push-test-miniapp@0.0.1"
  ]
},

we should probably update the schema in this more proper structure :

"container": {
  "nativeDeps": [
    "react-native@0.50.1",
    "react-native-code-push@5.1.3-beta"
  ],
  "miniApps": {
    "code-push-test-miniapp@0.0.1"
  }
}
,
"codePush": {
  "Staging": [
    {
      "metadata": {
      },
      "miniapps": [
        "code-push-test-miniapp@0.0.19"
      ]
    }
  ]
}

which make it easier to see (and work with in code) what is included in the container v.s what is included in codePush releases.

especially if we add a new jsApiImpls array, it will just be "plugged" at the right locations :

"container": {
  "nativeDeps": [
    "react-native@0.50.1",
    "react-native-code-push@5.1.3-beta"
  ],
  "miniApps": {
    "code-push-test-miniapp@0.0.1"
  },
  "jsApiImpls": {
    "react-native-ernmovie-api-impl-js@0.0.4"
  }
}
,
"codePush": {
  "Staging": [
    {
      "metadata": {
      },
      "miniApps": [
        "code-push-test-miniapp@0.0.19"
      ],
     "jsApiImpls": [
        "react-native-ernmovie-api-impl-js@0.0.4"
      ]
    }
  ]
}

So a new issue should be created first to refactor Cauldron structure appropriately and it should be made a pre-requisite for this current issue (which will then be around adding the jsApiImpls entries support in Cauldron / platform).

deepueg commented 6 years ago

yes, it may not be a good idea to store js under native dependencies. But from the top level, the command can still be cauldron add dependencies and the internal logic can figure if it is native or jsApi impls and change the cauldron structure as above.

IMO, introducing a new command option for JsApiImpl may add more confusion to cauldron users. Also, this needs to be considered for the following cases as well

  1. ern create-contianer --dependencies for local container generation without a cauldron
  2. A miniapp that contains a JSImpl as dependency.
belemaire commented 6 years ago

Depends on https://github.com/electrode-io/electrode-native/issues/395