New JSON structure - Githubissues

fernandojsg commented 8 years ago

Following the discussion we had (@dmarcos, @ngokevin) about it last week...

The current structure is:

{
  "collider" : {
    "author": "Diego Marcos",
    "description": "Triggers events when object collide",
    "url": "https://rawgit.com/dmarcos/a-invaders/31f5a7/js/components/collider.js"
  },
  "explode" : {
    "author": "Diego Marcos",
    "description": "Makes the geometry explode",
    "url": "https://rawgit.com/dmarcos/a-invaders/31f5a7/js/components/explode.js"
  },
  "spawner" : {
    "author": "Diego Marcos",
    "description": "Spawns an entity on an event",
    "url": "https://rawgit.com/dmarcos/a-invaders/31f5a7/js/components/spawner.js"
  }
}

Add some new parameters:

help_url: (optional) It will be used in the help button next to the inspector's sidebar.
screenshot_url: (optional) It will be shown in component list in the "add new component" modal dialog.
url: (mandatory) pairs of version and url: {"0.3.0": "http://blabla", "0.4.0": "http://blabla4"}
schema_grouping: (optional): If defined it will be used to group and sort the parameters on the inspector UI as proposed on https://github.com/aframevr/aframe-inspector/issues/303. We could even force to hide the parameters from the schema not defined in this grouping, we could get easily get rid of parameters like mergeTo or similar.

With the changes proposed the JSON could looks like the following:

{
  "collider": {
    "author": "Diego Marcos",
    "description": "Triggers events when object collide",
    "url": {
      "0.3.0": "http://blabla",
      "0.4.0": "http://blabla4"
    },
    "help_url": "http://helpurl",
    "screenshot_url": "http://screenshot.png",
    "schema_grouping": [
      {
        "name": "size",
        "attributes": [
          "width",
          "height",
          "width"
        ]
      },
      {
        "name": "segments",
        "attributes": [
          "segmentwidth",
          "segmentheight",
          "segmentwidth"
        ]
      }
    ]
  },
  "explode": {
    "author": "Diego Marcos",
    "description": "Makes the geometry explode",
    "url": {
      "0.3.0": "http://blabla"
    },
    "help_url": "http://helpurl"
  },
  "spawner": {
    "author": "Diego Marcos",
    "description": "Spawns an entity on an event",
    "url": {
      "0.3.0": "http://blabla"
    }
  }
}

fernandojsg commented 8 years ago

We could even add a group to group the components as Unity does. Something like: Models, Effects, Physics, Audio, etc.

dmarcos commented 8 years ago

You could also add a tags attribute.

dmarcos commented 8 years ago

If we are going to add property grouping it should probably be part of the schema itself and not this JSON file

fernandojsg commented 8 years ago

@dmarcos :+1: for tags re grouping it's not needed in aframe at all, is just about the representation in the inspector, and let's say that you could for example add type of widget for each attribute in the future, these are just inspector's features why would we want to add them in aframe?

dmarcos commented 8 years ago

You have schema information scattered in multiple places. What happens if you modify / remove component properties? You have to also modify the schema_grouping. How do you handle multiple versions of the components? They might have different properties / groupings.

fernandojsg commented 8 years ago

I agree that having this grouping in AFRAME will be less error prone and more easy to keep in sync with the changes in the schema. My only concern is about including features on the core that are going to be used just by the inspector and not by the core itself.

ngokevin commented 8 years ago

The grouping seems to be purely presentational and should be kept out of the core. Else we couple A-Frame to the Inspector when the Inspector should have been a clean abstraction layer on top of A-Frame.

IMO, we should just leave out grouping and stay with simple alphabetical order. Alphebetical order ensures consistency between presentation of all components. Grouping adds maintenance and potential for version mismatches against this field.

fernandojsg commented 8 years ago

I agree that we should try not to include any inspector specific feature in the core that's why I proposed to do it here. Regarding using or not grouping, we'll leave it as optional, but I believe it's quite useful even if it need some maintenance, just take a look at the drafts that @feiss did here https://github.com/aframevr/aframe-inspector/issues/303. And for example with the new material changes with plenty of new options, I believe it will be nice to have the parameters grouping by type of maps for example

ngokevin commented 8 years ago

I think we can simplify by not having the JSON be aware of different A-Frame versions. The JSON itself could be tagged/versioned. That way you don't have to worry about maintaining multiple metadata for different A-Frame versions of a component in the same file.

https://github.com/aframe-components/tree/v0.3.0/components.json

https://github.com/aframe-components/tree/v0.4.0/components.json

fernandojsg commented 8 years ago

And what about the case when we just release the new aframe version let's say 0.4.0 and people still didn't try it with theirs components? Should ge load all the versions (0.3.0) of the aframe-components and expose them with some warning saying that we can't guarantee that this will work with the current version?

ngokevin commented 8 years ago

We can perhaps have a build step to concatenate all the tags to get:

{
  "0.3.0": {

  },
  "0.4.0": {

  }
}

This provides some information on the minimum version a component was known to support. We could provide a compat warning on 0.4.0 if a version supported 0.3.0 but nothing was explicitly updated for 0.4.0. Any components that require an update to work can register themselves with the new version of the JSON.

This might mean that every upgrade, we have a fresh working JSON for current version, and a built JSON consisting of all older versions.

ngokevin commented 8 years ago

Unrelated: we could also have build steps to pre-populate fields from NPM (or package.json found on GitHub).

ngokevin commented 8 years ago

I believe the system we are creating mirrors Firefox add-ons:

A-Frame Versions + Components <=> Firefox Versions + Add-ons

Here is how they handle compatibility: https://developer.mozilla.org/en-US/docs/Extension_Versioning,_Update_and_Compatibility

donmccurdy commented 8 years ago

Most of the URLs here include either a version number or a SHA1 – that's confusing to me for two reasons:

I'm happy to initially register components I've authored, but writing a PR to this repo every time I bump the version sounds tedious. Would the URLs somehow automatically update, and if so where's a permanent identifier in the JSON to allow that? Or do we create aframe publish-like CLI tooling?
If anything, I would expect component versioning to be associated with a given scene, not global to the inspector.

I recognize that NPM itself doesn't do everything we need, but I wonder if component versioning and registration could build on its APIs? The JSON here could theoretically be stripped down as far as:

[
  'aframe-collider',
  'aframe-explode',
  'aframe-spawner'
]

And then use something like npms-api to pull package metadata ~daily and pick up the most recent version, its URL, and popularity data: https://api.npms.io/module/aframe-leap-hands. Or grab the package.json from npmcdn: https://npmcdn.com/aframe-leap-hands/package.json

From package.json alone, a lot can be determined. A-Frame version compatibility should be a peerDependency. If a component depends on physics, that should also be a peer dependency. We could add aframe-specific fields there, as Browserify does:

// package.json
{
"name": "aframe-leap-hands",
"author": "Don McCurdy <dm@donmccurdy.com>",
"version": "0.5.1",
"description": "Leap Motion component for A-Frame VR",         // generic description
"peerDependencies": {
  "aframe": "^0.3.1"
},
"aframe": [{
  "type": component | system | primitive,
  "name": "leap-hands",
  "description": "Displays a skeletal hand using Leap Motion", // aframe-specific description
  "dist": "dist/bundle.min.js",
  "rating": comfortable | moderate | intense | non-vr,
  "tags": ["input", "physics"]
}]
}

Some new .aframe file could be an alternative to package.json.

EDIT: Not to say that everything I mention should be supported, but just that this is an extensible way to declare the metadata.

fernandojsg commented 8 years ago

I'm not an expert on npm and its api but as far I can understand It seems to be a nice idea what @donmccurdy proposes. I don't understand how to deal with the aframe version dependencies. For example if you've updated your latest version of aframe-leap-hands and we access the package.json it could has peerDependency of aframe 0.4.0, but you had a compatible version with aframe 0.3.0. How can we find them? My main concern is about the entry step for people that are not that much into npm and just want to publish their components in an easy way. What do you think @ngokevin @dmarcos ?

ngokevin commented 8 years ago

You could publish your component to NPM, and use a service like unpkg.io (formerly npmcdn).

https://unpkg.com/

This allows you to get a URL like unpkg.com/aframe-leap-hands/^0.3.0/dist/bundle.min.js. Notice the ^0.3.0. You can define versions just like you would in a package.json or semver, and it'll fetch the appropriate version. So if you bump the patch version, it'll automatically be reflected live for anyone using that URL.

ngokevin commented 8 years ago

I do like reusing package.json though.

fernandojsg commented 8 years ago

But if you use the URL unpkg.com/donmccurdy/aframe-leap-hands/^0.3.0/dist/bundle.min.js you're accessing version 0.3.0 of aframe-leap-hands right? But you're not defining the version of its aframe dependency. So we're assuming that people should use the same versions that the compatible aframe versions right?

+1 for reusing package.json

ngokevin commented 8 years ago

The URL will access the latest version up until the latest major version. So 0.3.x would probably be safer in that case. Once we jump A-Frame to major, then people can start doing ^4.0.0.

I think the policy should be similar to Firefox add-ons. When upgrading to the next A-Frame version, attempt to make all the previous ones work, with a large disclaimer. If a new ("major") version is uploaded, then it will override the previous one. So you sort of have a cascading effect.

fernandojsg commented 8 years ago

Ok, I was thinking in the case where the component's versions are not related to aframe's versions, let's say that the aframe-leap-hands compatible version for aframe 0.4.0 is 10.3.2, but if we enforce using the same versions for aframe and its components they could just use things like 0.3.123 and it will match 0.3.x

ngokevin commented 8 years ago

The URL is mostly for developer convenience so they can push patches without having to PR. Not related to how we version.

I think the Firefox add-on model still applies, it can work even if the numbers don't match. We just have to know the last minimum working version.

fernandojsg commented 8 years ago

Ok. So the workflow will be something like this?:

The user creates a component, push it to npm
Creates a PR to this repo to include the package name
Keep updating the component and pushing changes to npm

And from the inspector's side:

Query this repo's json to check the available components.
Query the api.npm or npmcdn to access package.json for metadata
Inject the script from unpkg.com using the version number from the scene's Aframe.

ngokevin commented 8 years ago

I don't think we should tie the version number of the component to A-Frame. We should keep track of that somewhere else. How about separate JSON for each version of A-Frame?

0.3.0.json
- physics ^4.1.0
- fireball ^1.0.0
0.4.0.json
5.0.0.json
- physics 5.0.0

Then we have a build step to process the JSONs with the following policy. Each new version of A-Frame assumes all of the same components from the previous version, but a disclaimer will be raised if there isn't an update component explicitly mentioned. Using the example above:

Use physics with 0.3.0 is OK
Use fireball with 0.3.0 is OK
Use physics with 0.4.0 is OK, but a warning will be raised
Use fireball with 0.4.0 is OK, but a warning will be raised
Use physics with 5.0.0 is OK
Use fireball with 5.0.0 is OK, but a warning will be raised

And perhaps an extra field to list components that we know do not work from the previous version.

0.3.0.json
- physics ^4.1.0
- fireball ^1.0.0
0.4.0.json
- exclude ocean
5.0.0.json
- physics 5.0.0

donmccurdy commented 8 years ago

The last part ("an extra field to list components that we know do not work from the previous version") sounds premature, I think. Maybe there is some way of codifying that, without requiring a description of the component at all historical revisions be maintained on the repo's HEAD?

// package.json
{
// ...
"aframe": {
    "type": component | system | primitive,
    "name": "leap-hands",
    "description": "Displays a skeletal hand using Leap Motion",
    "dist": "dist/bundle.min.js",
    "history": {  // a-frame version → component version
      "^0.2.0": "^1.5.0",
      "^0.3.0": "^1.6.0",
      "^0.4.0": "stable",
    }
  }
}

The build process would fetch master/package.json, and then v*.*.*/package.json for whatever subset of versions the editor should support.

ngokevin commented 8 years ago

A problem I see is having to chase down authors to update their package.jsons and publish them to NPM. Ideally, we can work without the back-and-forth. Perhaps there could be a combination of both methods where one method is a fallback.

I was thinking we'd usually only actively maintain the latest version's JSON, and maybe a tiny bit of the previous version. The rest live at the HEAD to ease the build step and so that everything is transparent.

Whatever we do, I like that policy that we should assume that components work on the next version. If they don't have an explicitly listed new version, we throw a warning. And if we somehow know a component doesn't work, then we have a way to exclude it until a new version is uploaded.

fernandojsg commented 8 years ago

In any case we should have some kind of caching for the component's info right? I mean instead of reading the array of npm-components-names and start fetching their package.json to generate the list of available components with a some info in the inspector.

ngokevin commented 8 years ago

You mean we cache fetched component metadata in the repo? Then yeah, we'd do that in the build steps.

fernandojsg commented 8 years ago

Yep exactly

dmarcos commented 8 years ago

If you cache the component metadata from npm you need to automate the process to do it periodically to make sure that the info is up to date.

dmarcos commented 8 years ago

To include components in the curated list... Is there a cheap way to do a community based curation a la hacker news? People upvote components published in NPM and there's a pre stablished criteria to include them in the "official" list. We would keep the criteria "secret" and "dynamic" to prevent people gaming the system.

fernandojsg commented 8 years ago

That could be an "v2" design for the current "do a PR with your new component and we'll review it". In any case we will end up having a list of components and the rest of the process will be as discussed before. So we could create a new issue with your proposal to mark it as "enhancement" and leave it for a new iteration when we already know that this is working and we have a huge amount of components

donmccurdy commented 8 years ago

People upvote components published in NPM and there's a pre stablished criteria to include them in the "official" list.

There is an open, unofficial API for NPM popularity data. Could manually review that, as you said, although NPM seems comfortable reporting "Packages people 'npm install' a lot" on their homepage, so maybe they validate things themselves.

fernandojsg commented 8 years ago

Cool, didn't know that!. What I mean about leaving it for "v2" is to try to get something easy to try as a POC for some packages and a manually review them so we could have something working already as a MVP and then iterate from there, instead of going directly for a "overengineering" solution right at the beginning.

ngokevin commented 8 years ago

Popularity could help us curate them, but I don't think the system should be based on popularity. One of the whole points of component discovery is to make well-built components more popular in the first place. We'd be missing out on lesser-known components that may be well-built.

fernandojsg commented 8 years ago

Following with our vidyo about the MVP: We agreed that we should need some metadata apart from the common package.json metadata. This metadata could be:

{
   "type": [component | system | primitive]
  "component-name": "leap-hands",
  "description": "Displays a skeletal hand using Leap Motion",
  "dist": "dist/bundle.min.js",
  "screenshot-url": [optional],
  "tags": ["locomotion", "controls"]
}

That data could be stored within the package.json custom attribute aframe, and then we could fetch each one of the packages to generate a cached json with all that metadata.

We could get rid of that and add a validation of that metadata by including it directly here. At the end that metadata is just specific from our curated list, it's not needed or affect anyhow the component outside from this use. So what I propose is to include this metadata at the same time the user does the PR. This metadata should remain the same on each version of the component as it doesn't include any reference to versions or commits, so they don't need to update every time they push to npm.

For the MVP we could have the whole list on one JSON, similar to:

[
  "kevin/leap-hands":
  {
    "type": [component | system | primitive]
    "component-name": "leap-hands",
    "description": "Displays a skeletal hand using Leap Motion",
    "dist": "dist/bundle.min.js",
    "screenshot-url": [optional],
    "tags": ["locomotion", "controls"]
  },
  "don/physics":
  {
    "type": [component | system | primitive]
    "component-name": "physics",
    "description": "Physics",
    "dist": "dist/bundle.min.js",
    "tags": []
  }
]

But later we could have a separated file for each component if the size increased a lot.

The version control will be handled as @ngokevin proposed by having a file for each version and we'll keep updating that file on each new aframe's release.

We'll use the component's json to show the data in the inspector without need to do any extra fetch or cache anything, and once we decide to include a component, we'll use the npm API to fetch the correct version of the file to inject.

What do you think?

dmarcos commented 8 years ago

it's going to be a correspondence between the json and the package of the version used?

fernandojsg commented 8 years ago

That will be handled on the versions' files: 0.3.0.json, 0.4.0.json where we could have something like:

physics: ^4.1.0
fireball: ^1.0.0

components.json: The curated list of components and the custom metadata that doesn't belong to a specific version.
versions/x.x.x.json: List of dependencies of each component compatible with this version. If a component that it's included on components.json is not included on this file, it means that it's not fully compatible yet, so we'll show a warning.
npm packages: We use the API to get the version we need based on the component's npm package name and the desired version from versions + AFRAME version of the scene.

dmarcos commented 8 years ago

do you want a file per version or just tagging the repo with the aframe versions?

fernandojsg commented 8 years ago

Umm I'm not sure, because once you tag the repo you can't modify it right? And sometimes you maybe keep increasing the aframe versions and the users fix compatibility issues with older versions at any time. How can we handle that? And what about the current version that it's still not tagged?

ngokevin commented 8 years ago

Here's my start: https://github.com/aframevr/aframe-components/pull/7

Yeah, for now, I think it's easiest for maintenance to keep a file per major version, and simplest for the build script if everything is not scattered across branches/tags.

fernandojsg commented 8 years ago

I like it! :) But don't you think we should need a components.json with every component that we've in our repo? Otherwise we should go through all the versions files to collect the components that maybe are not yet tested for the current version. Let's say we've physics on 0.3.x but we move to 0.4.0 and it's empty. To show physics on the list of available components (with warning) we should iterate through all the versions. If we've a list of components.json with every component on our repo, we could show the list of components and add a warning for everyone that is not on the current version file.

Also we could get rid of the duplications of name attribute for each component as I believe we should advice people not to change between versions

ngokevin commented 8 years ago

Hm, yeah, that makes sense to be able to see a single component's support throughout the versions in one place. Something like.

// registry.json
{
  "components": {
    "aframe-bmfont-text-component": {
      "name": "bmfont-text",
      "aframeVersion": {
        "0.3.0": {
          version: "0.1.x",
          path: "dist/aframe-bmfont-text-component.min.js"
        },
        "4.0.0": {
          version: null  // Means we found it does NOT work. Needs an update.
        }
      }
    }
  }
}

ngokevin commented 8 years ago

There's the npm name and the component name. We need the npm name to figure out where to fetch metadata. We need the component name for like...displaying in the Inspector dropdown, and this information isn't available through npm yet (unless we try to parse the JS file).

fernandojsg commented 8 years ago

Yep, I agree with the component/npm name, I was wondering how to prevent the duplication across the versions files. re the registry.json, Yep, that could be nice. Or even just remove the whole 4.0.0 key in case that is not supported yet and we could just query for that version and see it's empty so just show a warning.

ngokevin commented 8 years ago

I was thinking if we don't list the version, we show the warning (it may or may not work).

If you do list the version, and set null, that means we know/confirmed it doesn't work.

fernandojsg commented 8 years ago

Yep, good point :+1:

fernandojsg commented 8 years ago

That will helps also to review the remaining components

donmccurdy commented 8 years ago

Is the component version, "0.1.x", using a wildcard? If so, is there a programmatic way to get the latest version matching that wildcard from GitHub/NPM without just making fetch requests in a for-loop?

ngokevin commented 8 years ago

Yeah, unpkg.com (formerly npmcdn) will give you the latest version if you use a fuzzy version.

https://unpkg.com/aframe-bmfont-text-component@*/dist/aframe-bmfont-text-component.min.js

ngokevin commented 8 years ago

the initial json builder completed at https://github.com/aframevr/aframe-registry/pull/7

We can file more issues to refine the policy or tweak the structure.

aframevr / aframe-registry

New JSON structure #5