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

json valueEncoding doesn't work as expected #495

Closed
huan opened this issue Sep 24, 2017 · 22 comments
Closed

json valueEncoding doesn't work as expected #495

huan opened this issue Sep 24, 2017 · 22 comments
Assignees
Labels
bug Something isn't working

Comments

@huan
Copy link
Contributor

huan commented Sep 24, 2017

I use json valueEncoding for v1 and it works without any problem.

After upgrade to v2, It does not work anymore: after putting an object to leveldb, we will get back a [object Object] value of string type.

The following code can be able to reproduce the problem:

import * as encoding   from 'encoding-down'
import * as leveldown  from 'leveldown'
import * as levelup    from 'levelup'

async function main() {
  const encoded = encoding(leveldown('/tmp/test'), {
    valueEncoding: 'json',
  })
  const levelDb = levelup(encoded)

  await levelDb.put('test', {a: 1})
  const value = await levelDb.get('test')

  console.log('value type:', typeof value)  // value type: string
  console.log('value:', value) // value: [object Object]
}

main()
"dependencies": {
    "encoding-down": "^2.2.1",
    "leveldown": "^1.8.0",
    "levelup": "^2.0.0-rc3",
}

After a little digging, I found the following code in AbstractLevelDOWN, which is just String(value) for the value.

AbstractLevelDOWN.prototype._serializeValue = function (value) {
  if (value == null) return ''
  return this._isBuffer(value) || process.browser ? value : String(value)
}

I have no idea how to fix it because there's too many modules/layers.

@ralphtheninja
Copy link
Member

Thanks for reporting this. We are aware of it. But nice to have a separate issue for it :)

@ralphtheninja ralphtheninja self-assigned this Sep 24, 2017
@ralphtheninja ralphtheninja changed the title json valueEncoding does work as expected json valueEncoding doesn't work as expected Oct 2, 2017
@ralphtheninja ralphtheninja added the bug Something isn't working label Oct 2, 2017
@ralphtheninja
Copy link
Member

Will fix this today or tomorrow. After that I think we should release 2.0.0.

@huan
Copy link
Contributor Author

huan commented Oct 2, 2017

Great to hear that, awesome!

@huan
Copy link
Contributor Author

huan commented Oct 2, 2017

Looking forward to the release 2.0.0...

It seems that my build will not succeed until v2.0.0 is published...

> flash-store@0.0.14 dist:es6to5 /home/zixia/git/flash-store
> tsc --out ./bundles/flash-store.umd.js --target es5 --allowJs bundles/flash-store.es6.umd.js --lib es6,dom

node_modules/levelup/lib/levelup.d.ts(113,16): error TS2694: Namespace '"/home/zixia/git/flash-store/node_modules/abstract-leveldown/index"' has no exported member 'LevelDOWN'.
node_modules/levelup/lib/levelup.d.ts(127,16): error TS2694: Namespace '"/home/zixia/git/flash-store/node_modules/abstract-leveldown/index"' has no exported member 'LevelDOWN'.

My current dependence version:

   "encoding-down": "^2.3.1",
   "leveldown": "^2.0.0",
   "levelup": "^2.0.0-rc3",

@ralphtheninja
Copy link
Member

@zixia Mind trying this out again? deferred-leveldown was recently updated to take care of this.

@huan
Copy link
Contributor Author

huan commented Oct 7, 2017

It seems still have problems.

I'm using the following dependences:

    "encoding-down": "^2.3.1",
    "leveldown": "^2.0.0",
    "levelup": "MeirionHughes/levelup",

After rm -fr node_modules && npm i, I ran the follow code to reproduce the problem:

import encoding   from 'encoding-down'
import leveldown  from 'leveldown'
import levelup    from 'levelup'

async function main() {
  const encoded = encoding(leveldown('/tmp/test'), {
    valueEncoding: 'json',
  })
  const levelDb = levelup(encoded)

  await levelDb.put('test', {a: 1})
  const value = await levelDb.get('test')

  console.log('value type:', typeof value)  // value type: string
  console.log('value:', value) // value: [object Object]
}

main()

Output:

$ ts-node t.ts
value type: string
value: [object Object]

The value type should be 'object'

@vweevers
Copy link
Member

vweevers commented Oct 7, 2017

@zixia I ran your example in node (with levelup 2.0.0-rc3), got:

$ node index.js
value type: object
value: { a: 1 }

@vweevers
Copy link
Member

vweevers commented Oct 7, 2017

Cannot reproduce with TS either. Seems to work fine here, and fails as expected if I downgrade deferred-leveldown to 2.0.0 2.0.1.

@huan
Copy link
Contributor Author

huan commented Oct 7, 2017

@vweevers I had tried again, but I still ran into the same buggy result.

I had pushed my configuration & source code at here: huan/flash-store@abfb02b

Travis CI result(failed) at here: https://travis-ci.org/zixia/flash-store/jobs/284564104#L762

    operator: equal
    expected: 'object'
    actual:   'string'
    at: Object.<anonymous> (/home/travis/build/zixia/flash-store/src/flash-store.spec.ts:154:5)

@vweevers
Copy link
Member

vweevers commented Oct 7, 2017

@zixia from the travis logs, seems it didn't install the latest deferred-leveldown:

levelup@2.0.0-rc3 (github:MeirionHughes/levelup#28131b7ffbf4834010833e06c42b4e9f55fca317)
│ ├─┬ deferred-leveldown@2.0.1

@huan
Copy link
Contributor Author

huan commented Oct 7, 2017

@vweevers Yes, you are right. After installing the deferred-leveldown@2.0.2, the test passed!

# levelup json bug
ok 19 value type should be object
ok 20 should get back the original object

Sorry for misreporting because I was thought the leveldown had already linked the latest version of it.

@vweevers
Copy link
Member

vweevers commented Oct 7, 2017

I'm not sure how the latest npm dedupes, we might need to depend on deferred-leveldown: ~2.0.2 instead of deferred-leveldown: ~2.0.0 to make sure it's installed. @ralphtheninja?

@ralphtheninja
Copy link
Member

@vweevers It can't hurt :)

@MeirionHughes
Copy link
Member

if @zixia is using yarn or npm 5, there will be a lockfile; I imagine it won't bump up versions unless you explicitly do so, or something depends on a specific patch version minimum.

@vweevers
Copy link
Member

vweevers commented Oct 7, 2017

right. damn lockfiles :)

@ralphtheninja
Copy link
Member

Maybe we should add tests for this.

@vweevers
Copy link
Member

vweevers commented Oct 7, 2017

To levelup you mean? +1

@ralphtheninja
Copy link
Member

Yes

@huan
Copy link
Contributor Author

huan commented Oct 7, 2017

There might have other problems with the version dependencies because I'm not using package-lock.json: I hate it and had already added it to the .gitignore.

+1 for unit test, and I'd like to suggest that we also add a TypeScript unit test in level* to make sure the typing is not broken.

@vweevers
Copy link
Member

vweevers commented Oct 7, 2017

I hate it

😁

and I'd like to suggest that we also add a TypeScript unit test in level* to make sure the typing is not broken.

Broken in what way?

@huan
Copy link
Contributor Author

huan commented Oct 8, 2017

It's just my 2 cents. I think if we could have a repository that integrated with all the level-* modules and have a build/test script automatically run by CI, after enabled greenkeeper auto update, it might be able to help us to monitor the comparable status of every module update on NPM. :)

@ralphtheninja
Copy link
Member

ralphtheninja commented Oct 8, 2017

Tests for this added in #508

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants