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

Expose methods as functions (bound methods) #86

Closed
Treora opened this issue Feb 2, 2017 · 5 comments · Fixed by #87
Closed

Expose methods as functions (bound methods) #86

Treora opened this issue Feb 2, 2017 · 5 comments · Fixed by #87

Comments

@Treora
Copy link

Treora commented Feb 2, 2017

Currently, importing a single function from a module (in ES6) like so:

import { getDomain } from 'tldjs';
getDomain(url)

fails with:

TypeError: Cannot read property 'isValid' of undefined
    at getDomain (.....)

This happens because getDomain relies on this being the tldjs object.

To fix this, could you perhaps expose the methods as independent functions?

@thom4parisot
Copy link
Owner

thom4parisot commented Feb 2, 2017

@Treora yeah it would be totally doable but it would imply to possibly break the API. Which is not a bad thing considering it is more a functional library rather than a class, really.

I see a few changes to operate:

  • this.validHosts is bound to the class instance, and is used by #getDomain and #isValid;
  • #getRulesForTld is used by #getDomain; it relies on this.rules to operate;
  • static methods can be taken out so that's an easy job.

Maybe the easiest move would be to:

  1. make everything as an independent function (some with more parameters)
  2. tldjs exports an object with partially applied rules (for convenience)

I suppose if people want to feed it different rules, they would have to do some more work but well, it can be documented or made easier for them if needed.

What do you think?

@Treora
Copy link
Author

Treora commented Feb 2, 2017

You could opt to keep the API exactly the same if desired: you could just pass each function through bind, so that its this will always be the tld instance.

If you wish to clean things up though, I would stop having rules and validHosts as global state of the module. Partially applied rules is indeed one way to go, or you could let the user 'recreate the module' with custom rules, for example like so:

function factory(rules, validHosts) {
    function getDomain(url) {
       ...rules[...url...]...
    }
    ...
    return {
        getDomain: getDomain,
        getSubdomain: getSubdomain,
        ...
        withCustomRules: factory
    }
}
module.exports = factory(require('rules.json'), [])

I have no strong opinion either way.

@thom4parisot
Copy link
Owner

@Treora, feel free to have a look at #86 to see if it fits your need. I had to change the way valid hosts were assigned as I don't think it is possible to track a mutated bound array.

@Treora
Copy link
Author

Treora commented Feb 13, 2017

Awesome. I almost feel guilty about having triggered such a code reorganisation effort with my tiny complaint. Thanks for your noble labour!

@thom4parisot
Copy link
Owner

No problem, it was some kind of distraction for the mind :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants