mapbox / tilelive

fast interface to tiles with pluggable backends - NOT ACTIVELY MAINTAINED
BSD 3-Clause "New" or "Revised" License
531 stars 108 forks source link

Update node version in tests #195

Closed mapsam closed 7 years ago

mapsam commented 7 years ago

Resolves #194

cc @tcql @springmeyer @rclark

mapsam commented 7 years ago

Looks like something must have changed with createWriteStream in node v6 where this test is now valid and does not throw. I can recreate this on my local.

rclark commented 7 years ago

The list stream generates a node.js Transform stream. A transform used to not be an instanceof a Writable stream. In node 6+ it now is

$ node 
> const stream = require('stream')
undefined
> new stream.Transform() instanceof stream.Transform
true
> new stream.Transform() instanceof stream.Writable
true
> new stream.Transform() instanceof stream.Readable
true

These tests and this conditional has always been lazy way of making sure you don't tilelive.createWriteStream({ type: 'list' }), which doesn't make sense. It ought to just be an assertion on the provided type instead.

mapsam commented 7 years ago

Thanks @rclark! Is this what you had in mind? https://github.com/mapbox/tilelive/pull/195/commits/ed0c1f2408f3d39e4fbad213c7c4289cdb49e558

I'm guessing the conditional should be the inverse, such as if (options.type !== 'list' || options.type !== 'something-else')?

mapsam commented 7 years ago

Per discussion with @springmeyer - going to keep 0.10.x in .travis for the time being, since this is what Mapbox Studio Classic and others are using.

mapsam commented 7 years ago

@rclark I've updated the test to create a ZXY stream for each before/after and asserting that the zxy values match. Happy to go one step deeper if we want to before.getTile(...) and after.getTile(...) if that's necessary.