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

feat: support custom serialize & deserialize through options #22

Merged

Conversation

tunnckoCore
Copy link
Contributor

Duh. Finished.

partially addresses and resolves #21

Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
index.js Outdated Show resolved Hide resolved
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
index.js Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@tunnckoCore
Copy link
Contributor Author

I'm on it. Sorry for the delay.

Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
@tunnckoCore
Copy link
Contributor Author

ooooh, of course... github is partially having problems.

package.json Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@tunnckoCore
Copy link
Contributor Author

@borisdiakur: I think the catch block should look like this

You probably meant if err is EEXIST then resolve()? Otherwise

if (err && err.code === 'EEXIST') {
    reject(err)
    return
  }
  reject(err)

doesn't make any sense :)

Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
@tunnckoCore
Copy link
Contributor Author

@borisdiakur I think I came up with good solution - adding options.throwError option which controls if it should error when the cache directory already exists.

The default behavior of error-ing remains when directory exists (and when there is invalid combination of cache dir and cache id), as seen in the tests. The other behavior is to pass options.throwError: false to disable it - e.g. not erroring, which I need because in my case i don't use cacheId and etc. and in the same time I don't care if the cache directory (cachePath) already exists.

Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
@tunnckoCore
Copy link
Contributor Author

@borisdiakur why there is serialize: 'qux', in 3 of the tests? Do you meant options.salt there?

Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
@borisdiakur
Copy link
Owner

@borisdiakur why there is serialize: 'qux', in 3 of the tests? Do you meant options.salt there?

🙈 Yes, it should have been salt.

Copy link
Owner

@borisdiakur borisdiakur left a comment

Choose a reason for hiding this comment

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

I just checked out the MR and run the tests and two of them are failing:

 2 failing

  1) memoize-fs
       check args
         should throw an error when opts.serialize is not a function when passed:
     AssertionError [ERR_ASSERTION]: The input did not match the regular expression /serialize option of type function expected/. Input:

'Error: options of type object expected'

      at Context.<anonymous> (test/test.js:20:14)
      at processImmediate (internal/timers.js:456:21)

  2) memoize-fs
       check args
         should throw an error when opts.deserialize is not a function when passed:
     AssertionError [ERR_ASSERTION]: The input did not match the regular expression /deserialize option of type function expected/. Input:

'Error: options of type object expected'

Can you check what is going on? We are almost there 🙂
I think you just need to rename object to function in the two tests.

Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
package.json Outdated Show resolved Hide resolved
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
@tunnckoCore
Copy link
Contributor Author

Updated the docs too.

Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
@tunnckoCore
Copy link
Contributor Author

Done. :) Hope it's great now! 🎉

@borisdiakur
Copy link
Owner

Done. :) Hope it's great now! 🎉

It’s almost great 😅

Regarding the --reporter=text-lcov: Can you have a look at https://github.com/borisdiakur/memoize-fs/pull/22/files#r385794490

Or I can also paste it here:

node ./node_modules/.bin/nyc report | node --trace-uncaught ./node_modules/.bin/coverallsfile ----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |                   
 index.js |     100 |      100 |     100 |     100 |                   
----------|---------|----------|---------|---------|-------------------

[error] "2020-02-28T16:42:08.814Z"  'error from lcovParse: ' 'Failed to parse string'
[error] "2020-02-28T16:42:08.814Z"  'input: ' '----------|---------|----------|---------|---------|-------------------\n' +
  'File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s \n' +
  '----------|---------|----------|---------|---------|-------------------\n' +
  'All files |     100 |      100 |     100 |     100 |                   \n' +
  ' index.js |     100 |      100 |     100 |     100 |                   \n' +
  '----------|---------|----------|---------|---------|-------------------\n'
[error] "2020-02-28T16:42:08.814Z"  'error from convertLcovToCoveralls'

/Users/m267843/workspace/open_source/memoize-fs/node_modules/coveralls/bin/coveralls.js:19
      throw err;
      ^
Failed to parse string
Thrown at:
    at /Users/m267843/workspace/open_source/memoize-fs/node_modules/coveralls/bin/coveralls.js:19:7
    at /Users/m267843/workspace/open_source/memoize-fs/node_modules/coveralls/lib/handleInput.js:21:9
    at /Users/m267843/workspace/open_source/memoize-fs/node_modules/coveralls/lib/convertLcovToCoveralls.js:60:14
    at walkFile (/Users/m267843/workspace/open_source/memoize-fs/node_modules/lcov-parse/lib/index.js:108:9)
    at /Users/m267843/workspace/open_source/memoize-fs/node_modules/lcov-parse/lib/index.js:116:20
    at suppressedCallback (fs.js:210:5)
    at fs.js:154:23

@tunnckoCore
Copy link
Contributor Author

tunnckoCore commented Feb 28, 2020

Aahh. That's... strange thing. But yea, the nyc's recipe is also doing it that way so.. Ok.

Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
@borisdiakur borisdiakur merged commit 31c20cb into borisdiakur:master Feb 28, 2020
@tunnckoCore tunnckoCore deleted the support-custom-serialize-deserialize branch February 28, 2020 17:26
@tunnckoCore
Copy link
Contributor Author

🎉 🌮

@borisdiakur
Copy link
Owner

Nice work @tunnckoCore ! 🚀

I’m adding a few (minor) final touches and will publish a new version soon. Thanks a lot for contributing! 🙏

@tunnckoCore
Copy link
Contributor Author

tunnckoCore commented Feb 28, 2020

Great! Thanks you too :) 🙌

I have more in the pocket ;p But that's okay for now.

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.

Make cache file be regular .js file
2 participants