Skip to content

Doesn't work with destructured arguments #10

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

Closed
TooTallNate opened this issue Sep 17, 2017 · 19 comments
Closed

Doesn't work with destructured arguments #10

TooTallNate opened this issue Sep 17, 2017 · 19 comments

Comments

@TooTallNate
Copy link

I'm not exactly sure what the format for destructured Arrays and Objects should be, but they're definitely not supported currently.

For example:

function foo([a, b], { c, d }) {
}

console.log(functionArguments(foo));

// actual
[ '[a', 'b]', '' ]

// possibly expected?
[ [ 'a', 'b', ], { c: true, d: true } ]

Thoughts?

@ORESoftware
Copy link

ORESoftware commented Sep 18, 2017

@TooTallNate maybe use this instead?
https://github.com/tunnckoCore/parse-function

I believe @charlike wrote the above, so all in the family...are you creating a DI library by the way? Curious what your use case is.

@tunnckoCore
Copy link
Owner

TooTallNate, yes it is not supported here, and won't be. This package is smallest and simple approach.

in parse-function is also not supported currently but it is exstensible with plugins and different parsers. there is open issue for what should be the result, because we are not sure too

@TooTallNate
Copy link
Author

Couldn't it remain simple, but still be updated to support ES6 syntax?

@TooTallNate
Copy link
Author

Related: tunnckoCore/parse-function#47

@TooTallNate
Copy link
Author

@ORESoftware

are you creating a DI library by the way?

Yes.

@tunnckoCore
Copy link
Owner

dont know, it is regex based so..

prs welcome anyway if it's someting small and dont add much code/deps.

@ORESoftware
Copy link

ORESoftware commented Sep 18, 2017

@TooTallNate uhh, well if you are creating a DI library, destructured args won't really work will it :)

think about it

function({a,b}){}

how iz yer DI lib gonna know what the name of the encompassing object is?

this will work of course:

function(c){
    const {a,b} = c;
}

@TooTallNate
Copy link
Author

TooTallNate commented Sep 18, 2017

The DI lib only needs to know that the user is using a and/or b and then supply the arguments. I wouldn't expect them to use c as a parameter name for this particular case (it would probably throw an Error).

@tunnckoCore
Copy link
Owner

tunnckoCore commented Sep 19, 2017

Hm, pretty easy implementation using balanced-match.

const balancedMatch = require('balanced-match')

function functionArguments (fn) {
  if (typeof fn !== 'function') {
    throw new TypeError('function-arguments expect a function')
  }
  if (!fn.length) {
    return []
  }
  let fnToStr = Function.prototype.toString
  let fnStr = fnToStr.call(fn)

  // from https://github.com/jrburke/requirejs
  let reComments = /(\/\*([\s\S]*?)\*\/|([^:]|^)\/\/(.*)$)/gm
  fnStr = fnStr.replace(reComments, '') || fnStr

  let res = balancedMatch('{', '}', fnStr)

  // when destructing arguments
  if (!res.post.length) {
    const pre = res.pre.replace(reComments, '') || res.pre
    res = balancedMatch('(', ')', pre)
  }

  if (!res.body.length) {
    return []
  }

  return res.body.split(',').map((str) => str.trim())
}

examples

const foo1 = (a, b, c) => {
  if (a) {
    return a + 1
  }
}

const foo2 = ({ a, b, c }) => {
  if (a) {
    return a + 1
  }
}

const foo3 = function (a, b, c) {
  if (a) {
    return a + 1
  }
}

const foo4 = function ({ a, b, c }) {
  if (a) {
    return a + 1
  }
}

console.log(functionArguments(foo1)) // => [ 'a', 'b', 'c' ]
console.log(functionArguments(foo2)) // => [ 'a', 'b', 'c' ]
console.log(functionArguments(foo3)) // => [ 'a', 'b', 'c' ]
console.log(functionArguments(foo4)) // => [ 'a', 'b', 'c' ]

Are these results okey? Using balanced-match (which i was thinking to use from the start) is easy and simplifies the things, but not sure for the performance and such.

Should we add an option to control this? For example destructuring as second argument, so it will return an array of destructed arguments only when is true?

@tunnckoCore
Copy link
Owner

tunnckoCore commented Sep 19, 2017

I think destructuring is pretty fantastic and perfect for enforcing conventions and i'm using it always, but not so good for DI, don't know.

@ORESoftware
Copy link

ORESoftware commented Sep 19, 2017

@TooTallNate sorry I didn't really follow what you meant...ok now I know what you mean. So it will see a and b, and then wrap those two in an object. But then I don't see what the point of destructuring args is then.

instead of this:

function({a,b}){}

just expect your users to use:

function(a,b){}

@ORESoftware
Copy link

ORESoftware commented Sep 20, 2017

amirite or amirite?

I ask because I am creating a DI lib too and interested in making it as usable as possible

I'd venture to guess 90% of people interested in function-arguments will be wanting to make DI possible

@TooTallNate
Copy link
Author

TooTallNate commented Sep 20, 2017

@ORESoftware Ya it's a good point. One of the benefits with the desctructured object approach is that you can destructure further in the function signature. So if I'm using user I can destructure it like:

function ({ user: { id, name } }) {
}

Whereas with named parameters I'd have to do it in another line:

function (user) {
  const { id, name } = user;
}

@ORESoftware
Copy link

Interesting, yeah if you find a way to get destructuring to work well with DI, I'd like to know, thanks

@ORESoftware
Copy link

ORESoftware commented Sep 20, 2017

Like I was saying above
I don't know how a DI lib of any kind would know that user is supposed to be injected when you do:

function ({ id, name }) {
}

it's not really possible to know that id and name are supposed to be extracted from the user object.

The way my DI lib works, is there is a list of injectables:

const injectables = {
  foo: Promise.resove({}),
  bar: {},
  baz: function(cb){ 
        setTimeout(function(){
         cb(null, {});                     
        }, 100)
      }
}

If my lib can't find an injectable by key, it will try to require() the value. The user configures the injectables themselves, and the main win is it can handle async deps, as indicated.

So now I know what to inject when I have:

function (foo, bar, baz) {
}

@tunnckoCore
Copy link
Owner

tunnckoCore commented Sep 20, 2017

That's why real parser is needed, and parse-function is perfect for this job, because has plugins. So anyone can extend further. And that's why i almost always close issues here, this one is meant to be so simple and small.

@TooTallNate
Copy link
Author

If you're going to close the issue I'd at least mention in the README that ES6 features will not be supported, by design. I agree that the AST approach will be easier to implement.

@tunnckoCore
Copy link
Owner

tunnckoCore commented Sep 20, 2017

I'd at least mention in the README that ES6 features will not be supported

Ah, yea, good point 💯

Okey, thanks for opening this issue, but please try to create plugin for parse-function and then we can list it in the readme :)

If you need some help or something, feel free to ask. I should add some notes to its readme soon, until then you can see the readme of v4.

@ORESoftware
Copy link

ORESoftware commented Oct 25, 2017

There's always this sort of pattern

 const {Test} = suman.init(module, {
   $inject: ['a', 'b', 'c']
 });

 Test.create(function($inject){
    const {a,b,c} = $inject;
 });

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

No branches or pull requests

3 participants