will-stone/mobx-cookie

`Options` params usage in `constructor` and `set`

Closed this issue · 2 comments

Hi @will-stone, thanks for the great work providing a reactive cookie for mobx with mobx-cookie.

Recently, I ran through the code as I need to provide a Typescript definition for this library (we have to use Typescript at work). And there is a minor thing I reckon we can do.

class MobxCookie {
  /**
   * @param {*} name cookie's name
   * @param {*} options (optional) options to send to js-cookie
   */
  constructor(name, options) {
    this.name = name // name of set cookie
    this.options = options // options to send to js-cookie
    this._timeout = null // internal timer

    extendObservable(this, {
      value: jsCookie.get(name) // observable to keep in-sync with cookie value
    })

    this._syncTimeout()
  }

Our constructor is currently accepting a second parameter as js-cookie options. However, I don't think we use this.options anywhere, instead, if someone want to set the option, he/she needs to do it everytime set function is called.

I reckon there are two separated ways we can improve this:

1.) Drop options as the second parameter of the constructor.

2.) set function will make use of this.options if no options param is passed to this function, if there is an options param passed, it will be merged and overriden the class options.

What do you think ? I am happy to submit a PR for this issue.

It looks like I removed the last use of this.options here: c5f9a60#diff-a27cdde3e6123d1f6fd9a1279fa39eceL50 I have no idea why I needed it 😄 That's another thing that needs adding to the project: tests!

Now you've got me thinking about this project, I do feel the want to improve it. Let me think about it. I may give it a go this weekend (no promises).

Removed this.options in #5