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

JS refactor #12

Merged
merged 22 commits into from
Aug 15, 2017
Merged

JS refactor #12

merged 22 commits into from
Aug 15, 2017

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented Aug 7, 2017

Refactoring mvt-fixtures to be in pure Node.js using the pbf module and allowing us to document the fixture templates.

Major changes

  • All fixtures are located in /lib/fixtures.js and exist as JSON objects that can be converted into protocol buffers using the mvtf.load('name') method, which returns a buffer. These fixtures all have JSDOC syntax so we can document out the individual fixtures for us to refer to in the future
  • Adds the vector-tile specification repo as a submodule so we can reference the .proto file and build schemas accordingly
  • Does not include a directory of actual protocol buffers, just the API methods to create them
  • removes the _reserved/ directory

What's next?

  • decide if we want to publish the generated protocol buffers or if we are satisfied with just the javascript interface to generate them when necessary? I'd think that we do want to publish them, so 🤔 about how to make sure they have been generated prior to publishing.
  • create all fixtures from v2.x and put them in here with documentation
  • signoff on architecture from @springmeyer

cc @springmeyer @joto @GretaCB @millzpaugh @flippmoke

FIXTURES.md Outdated

> A feature MAY contain an id field. If a feature has an id field, the value
> of the id SHOULD be unique among the features of the parent layer
> ([reference](https://github.com/mapbox/vector-tile-spec/blob/master/2.1/README.md#42-features))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move descriptions for each fixture into the fixture, per We should think about some way of structuring the test vector at mapbox/vector-tile#20 (comment). We could do a folder per fixture, and then key files inside could:

  • describe it
  • have the code to generate it
  • have the fixture itself

See https://github.com/osmcode/osm-testdata/tree/master/grid/data/3/302 for inspiration.

@springmeyer
Copy link
Contributor

@mapsam - awesome work: this really feels like a great step forward. Nice work on using pbf!

Architecture wise, my thoughts are that we'll want to make the fixtures, and their metadata, accessible from other applications. We can either do that by structuring them, and storing them, inside this repo in a way that makes them easily accessible by others. Or we can publish them externally in some easy to use package and layout. Like you I am leaning towards storing them inside this repo.

My comment at https://github.com/mapbox/mvt-fixtures/pull/12/files#r131783630 mentions the idea of using subdirectories that would hold both the code to generate and the fixture. If we did this then the code in lib/ would be restricted to helper functions and the code in each fixture directory could use those helper functions to do simple things or be customized to do narly things (when it comes to making invalid fixtures).

Here is an idea: we could have a single script to re-generate all fixtures. It could:

  • loop through each fixture directory
  • if only a .json file exists, it would generate an .mvt from that json
  • if a .js file exists, it would run the .js and that script would output the .mvt and serialize a .json.

@mapsam
Copy link
Contributor Author

mapsam commented Aug 8, 2017

Thanks @springmeyer!

Happy to move fixtures into their own directories, or at least their own files. I'm a little hesitant to start creating four files per fixture, largely because it's adds extra clunkiness when adding new fixtures. Would you mind explaining the use case where metadata would be required by another application?

What about something like this, where each fixture is represented by its own module:

lib/fixtures/valid-single-point-no-id.js

module.exports = {
  name: 'valid-single-point-no-id',
  description: 'Has a single feature without an ID...',
  specification_reference: 'https://github.com/mapbox/vector-tile-spec/blob/master/2.1/README.md#42-features'
  json: {
    layers: [
      {
        version: 2,
        name: 'hello',
        features: [
          {
            // without id
            // id: 1,
            tags: [],
            type: mvt.Tile.GeomType.POINT,
            geometry: [ 9, 50, 34 ]
          }
        ],
        keys: {},
        values: {},
        extent: 4096
      }
    ]
  },
  manipulate: function() {
    // some sort of javascript manipulation
  }
}

This would allow us to loop through fixtures based on a fs.readDir (which is kinda ugly, but unavoidable if we're breaking apart into files) but prevent more file manipulation and keep the logic within javascript, like:

var buffer = createBuffer(fixture.json)
buffer = (fixture.manipulate) ? fixture.manipulate(buffer) : buffer;
// write file to system, or or return buffer

As far as doc generation, we can create some sort of markdown generator script instead of using documentation.js for the fixture docs.

@mapsam
Copy link
Contributor Author

mapsam commented Aug 8, 2017

I'm waffling, esp now thinking about where .mvt files live, it feels helpful to have a description of the file living next to that fixture. Could we programmatically generate a fixtures folder that has a .mvt and description.txt file, while keeping the source in a module like the one I described above? Example directory structure:

mvt-fixtures/
  |- index.js (helpers)
  |- lib/
    |- fixtures/
      |- valid-single-point-no-id.js
  |- fixtures (all generated programmatically)
    |- valid-single-point-no-id/
      |- tile.mvt 
      |- description.txt
  |- test/

@springmeyer
Copy link
Contributor

Would you mind explaining the use case where metadata would be required by another application?

Yes. My idea of having things in folders is to make them accessible for repos written in a language that is not JS. I am imagining that if the fixture directories had sidecar metadata, then both https://github.com/mapbox/vector-tile-js and https://github.com/mapbox/vector-tile and https://github.com/mapbox/vector-tile-cs, for example, could write a loop in JS, C++, and C# that looks somewhat like:

https://gist.github.com/springmeyer/1c4aee81fa57b6771e636c81683e9859

That would make it possible to use all the fixtures automatically, across many repos. The blue sky scenario would be: add a new fixture, release a new version of mvt-fixtures, update the version in your decoding lib, and (hopefully, mostly) automatically get the benefit of the new fixture.

Of course updating all libraries to have looping fixture tests would be a big lift. In that lift you'd need to consider issues like how to display/debug failures and skip them in certain cases. I'm just proposing here a design that would make it possible. The two key things I think are needed to make it possible are:

Could we programmatically generate a fixtures folder that has a .mvt and description.txt file, while keeping the source in a module like the one I described above?

👍

@mapsam
Copy link
Contributor Author

mapsam commented Aug 8, 2017

Cool - I like that @springmeyer. Let's make sure the following workflows are covered in code (via helper functions for JS users) or in the README via example docs (for non JS users):

  • Need a single test fixture for a single test (in JS, via helper function)
  • Need a single test fixture for a single test (not in JS, no helper function)
  • Loop through all test fixtures and provide JSON representations, buffers, and descriptions (in JS, via a helper function)
  • loop through all test fixtures and provided the same (not in JS, no helper function)

And development workflow for this library:

  • update source fixture JS files (or add a new one)
  • script that generates the following files:
    • a folder for each fixture in /fixtures/:name
    • a name.mvt file
    • a info.json file with description information
    • a name.json file that represents the data in the tile
  • write some sort of test/pre-release script that ensures all fixtures have been generated prior to tagging/releasing on npm - this could just be a test to assert whether a new fixture.js file also exists as a directory as described above
  • all new fixtures should be a minor version bump, any directory restructures should be a major version bump

@springmeyer
Copy link
Contributor

👍 to your layout proposal. I like the idea of clearly distinguishing docs to create vs read the fixtures. One minor thought is that the naming could be simplified to avoid needing to repeat :name:

  • a folder for each fixture in /fixtures/:name
  • a tile.mvt file
  • a info.json file with description information
  • a tile.json file that represents the data in the tile

@mapsam
Copy link
Contributor Author

mapsam commented Aug 9, 2017

Okay @springmeyer - I've updated this to start building all fixtures (including .mvt, info, and json) in the /fixtures directory - all source fixtures are located in /src. The ./index.js file has the following methods:

  • .get() which gets a single fixture
  • .each() which loops through every fixture
  • .create() which creates a fixture buffer from a custom JSON (in case one doesn't exist in the fixtures list)
  • .schema is the schema module produced by PBF

The following build scripts have been created:

  • /scripts/docs.js - generates FIXTURE.md docs
  • /scripts/build.js - generates all raw fixture files from /src into /fixtures
  • /scripts/new.js - creates a new /src fixture module template

Added tests that:

  • test all methods
  • test all fixtures, source and raw, for matching data, which ensures they have been generated prior to release
  • test buffers from FIXTURES.md and API.md to make sure they are up-to-date before merging PRs

@mapsam
Copy link
Contributor Author

mapsam commented Aug 9, 2017

Also, I'd recommend just viewing the branch instead of the diffs in this PR 😄 https://github.com/mapbox/mvt-fixtures/tree/refactor-js

@mapsam
Copy link
Contributor Author

mapsam commented Aug 11, 2017

@springmeyer
Copy link
Contributor

Great, I've commented on both of those tickets @mapsam. I feel like this is ready to merge into master either before or after those land once the tests are passing.

Looks like the tests are failing for node v6 due to vtinfo not yet providing node v6 binaries, which I presume is one of your motivations of mapbox/vtinfo#19. 👍 Looks to me like you could quickly use https://github.com/mapbox/vector-tile-js in place of vtinfo for the test usage currently that checks layer count/names.

@mapsam
Copy link
Contributor Author

mapsam commented Aug 14, 2017

Good idea @springmeyer - removed vtinfo and using vector-tile-js as a dev dependency.

@mapsam
Copy link
Contributor Author

mapsam commented Aug 15, 2017

@springmeyer mind giving the most recent changes an 👀 ? They include:

test/get.test.js Outdated
assert.ok(fixture.description);
assert.ok(fixture.specification_reference);
assert.ok(fixture.validity);
console.log(fixture.manipulate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this console.log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed!

Copy link
Contributor

@springmeyer springmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mapsam - this is looking great. I feel like it is ready to have others use to create more fixtures. I cannot think of any more adjustments needed at this point.

@mapsam mapsam merged commit 0cf34e4 into master Aug 15, 2017
@mapsam mapsam deleted the refactor-js branch August 15, 2017 21:12
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.

2 participants