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

[WIP] Separate Javascript schema API from Schema Specification #435

Open
wants to merge 1 commit into
base: schema-cleanup
Choose a base branch
from
Open

[WIP] Separate Javascript schema API from Schema Specification #435

wants to merge 1 commit into from

Conversation

bkp7
Copy link
Contributor

@bkp7 bkp7 commented Feb 15, 2018

PR for issue #389.

The api has been separated out and published on npm @bkp7/schema-js-api for now.

This PR references version 0.0.5 of @bkp7/schema-js-api which is basically the code cut out of this repository as is.

Subsequently, I have added some tests, fixed some issues and started on some documentation for the API which is currently at 0.0.7. There is a need, having separated out the API, to be able to load different versions of the schema as required, including local. As a result there is a small change to the API with .schemas being replaced with .getSchemas(). With this small change applied I am ensuring that later versions of the API work as expected with specification.

The API is currently light on documentation, as it never had to stand alone before. Before spending too much time tidying up the API I'd appreciate knowing that this will be adopted. There is no particular reason that the split shouldn't happen on the code as is, since the API is no less documented than it was previously.

@tkurki tkurki changed the title WIP: separate Javascript schema API from Schema Specification [WIP] Separate Javascript schema API from Schema Specification Feb 16, 2018
@tkurki
Copy link
Member

tkurki commented Feb 24, 2018

I would have approached this myself by extracting the js api first within the same repository, but no matter.

So how do things work now, when this repo's tests need the js api and the js api should use the schemas in this repo? I looked at the code briefly - it loads the published schema, right?

How about restructuring the js api so that it has a separate entrypoint for validation/schema related things? Something like require('@signalk/api').schemaApi(require('@signalk/signalk-schema')).

@bkp7
Copy link
Contributor Author

bkp7 commented Feb 27, 2018

Currently the separated api uses a local copy of the schema if it's available, otherwise http://signalk.org/specification/1.0.0/schemas/signalk.json. This allows both sets of testing, but yes, it needs to be changed!

Seems to me we need to be able to load:

  • any published version
  • a local version
  • possibly any version in github?

As regards published versions can we expose a list somewhere? A directory listing at signalk.org/specification/ ? I know that's where the playground is at the moment but it would be good if it could be self documenting. Maybe FTP?

Also, it would be useful to have a list of top level schemas automatically available for each version. Is the schemas folder structured the way it is for a reason, or would it be better to only have top level schemas in that folder? That way it would also be self documenting and would enable a list of schemas to be read. This would be useful for drop downs etc.

Agreed re separate entry point. My plan was to separate the api, document it, and then look at its structure and plan a way forward. I was reluctant to change much as I wasn't sure how many projects use the api. I'm currently aware of:

  • specification
  • playground
  • signalk-aishub-ws
  • @signalk/nmea0183-signalk
  • n2k-signalk
  • signalk-server
  • @signalk/zones

@fabdrol
Copy link
Member

fabdrol commented Jun 12, 2018

@bkp7 what's the beat on this PR?

@bkp7
Copy link
Contributor Author

bkp7 commented Jun 12, 2018

@fabdrol, we need to establish whether there are any other sets of code using the API which will need to be modified to use the separated API. If you are aware of any...

It's my next thing to look at to deal with the conflicts as it would be good to get it separated from the spec. prior to doing the schema cleanup.

@fabdrol
Copy link
Member

fabdrol commented Jun 12, 2018

@bkp7 I've been playing with some modules that show dependants.. different output and may not even be complete, due to the fact that some check the current version, and one checks every other version except current.. anyway, I've managed to find these so far (in addition to yours, may be some overlap)

signalk-aishub-ws, signalk-pebble-mydata, n2k-signalk, signalk-server, n2k-signalk, signalk-testclient, kgauge, signalk-polar-graphing

…o that new repo in npm (@bkp7/schema-js-api v0.0.5) which will need to be transferred to signalK ownership.

Before merging, all refs to @bkp7 need to be changed to @SignalK.
@bkp7
Copy link
Contributor Author

bkp7 commented Jun 13, 2018

@fabdrol, I've rebased to master so this should be good to implement.

The first thing we need to do is get the github and npm repository for the api code set up/transferred to be part of the SignalK project. Once that's done we can put a PR into each dependent project to point at the new api code. As stated above 0.0.5 is designed to be a simple reference replacement for what is present now. Perhaps easiest to contact me direct to implement transfer.

@fabdrol
Copy link
Member

fabdrol commented Jun 13, 2018

@bkp7 thanks, let's get a few more opinions before we move on with this one though. It's a consequential change, and we should probably do the changes to the dependant projects around the same time (at least the parsers, @SignalK projects, etc)

@SignalK/core input?

@tkurki
Copy link
Member

tkurki commented Jun 13, 2018

How about

  1. create the new api in a new repo
  2. change specification to use the new api (part of this PR, but from @signalk/schema-js-api)
  3. change the dependents
  4. deprecate & remove the old one (part of this PR)

These do not need to happen in lockstep or synchronously. The only danger is to leave some of the steps dangling.

@bkp7
Copy link
Contributor Author

bkp7 commented Jun 13, 2018

I've found 91 files in github (search: signalk/signalk-schema language:JavaScript) across an unknown number of repositories which reference @signalk/signalk-schema.

Most of them I've looked at (about 8) use a fixed version reference eg. "1.0.3". Some use the caret eg. "^1.0.3".

I believe that this PR needs to push the specification version number to 2.0.0 and that way each dependent app can change their refs as/when they see fit (we should provide documentation as to what's required).

Therefore I propose:

  1. create the new api in a new repo
  2. note the api in the 1.0.x documentation as being deprecated and advise to use the new api repo.
  3. apply this PR to the schema cleanup branch which will also require a major version bump anyway and because one of the objectives of cleanup is to lose the TV4 dependency which will require changes to the api.

@bkp7 bkp7 mentioned this pull request Jun 15, 2018
1 task
@fabdrol fabdrol changed the base branch from master to schema-cleanup July 11, 2018 07:39
@fabdrol
Copy link
Member

fabdrol commented Jul 11, 2018

@bkp7 I can merge this into schema-cleanup if you think it's ready for that, that'll just leave the other to things todo.

@bkp7
Copy link
Contributor Author

bkp7 commented Jul 13, 2018

@fabdrol, no please don't merge yet - it will need modifying to point to the new location. What I'm proposing is that we separate the api and use it in the master whilst leaving the deprecated api files in place within the master (suitably annotated) so that other projects won't break. That would just leave deleting the deprecated files to be done in schema-cleanup.

That being the case, could you setup/move @bkp7/schema-js-api to @signalk/schema-js-api and get the Travis, coveralls and Scrutinizer accounts linked up, or give me permissions/access to do that?

@fabdrol
Copy link
Member

fabdrol commented Jun 5, 2019

@bkp7 last year you mentioned that you didn't want this merged yet as there was still work to do. Any progress? Or should we close due tot lack of activity, make a new issue for it and look at it again later?

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

Successfully merging this pull request may close these issues.

3 participants