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

Error in the "Unknown Node" test and Vows runner hangs #24

Closed
rclayton-the-terrible opened this Issue Feb 24, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@rclayton-the-terrible

rclayton-the-terrible commented Feb 24, 2018

There's a slight syntax error here (you are missing a comma): https://github.com/marcelklehr/toposort/blob/master/test.js#L85

'unknown nodes in edges':
  { topic: function() {
      return toposort.array(['bla']  // this is the culprit
      [ ["foo", 'bar']
      , ["bar", "ron"]
      , ["john", "bar"]
      , ["tom", "john"]
      , ["ron", "tom"]
      ])
    }
  , 'should throw an exception': function(_, val) {
      console.log(val);
      assert.instanceOf(val, Error)
    }
  }

The actual error being caught is TypeError: Cannot read property 'length' of undefined when it should be something about the "Unknown Node".

When I add the comma to where it's supposed to go, I'm not receiving an error: expected [ 'bla' ] to be an instance of Error. Looking at the code, I'm not even sure an "unknown node" error is even possible since the check for the node's existence (!~nodes.indexOf(node)) is done in a loop over the node array you are making checks against (in pseudo code, it's something like for (node in nodes) if (!nodes.includes(node)) throw new Error() ).

I should also add that the Vows runner for this particular version is hanging for me:

screen shot 2018-02-23 at 7 36 30 pm

I would be happy to contribute a fix, however, I'm currently working against @supersam654's version, where (for whatever reason) the Vows runner terminates correctly:

screen shot 2018-02-23 at 7 37 50 pm

If you accept @supersam654's PR, I would like to submit a PR with custom errors. I'm building a Dependency Injection framework and I need to know which node fails the Cyclic Dependency check (and I don't want to parse the error message).

By the way, thanks for your work with toposort. I was previously working with Hapi's topo framework and I find your library to be much cleaner.

@marcelklehr

This comment has been minimized.

Owner

marcelklehr commented Apr 28, 2018

The vows runner was "hanging" because the previous version was just really slow for large graphs.

A property on the error object containing the cyclic dep node may be a good idea!

@marcelklehr

This comment has been minimized.

Owner

marcelklehr commented Apr 28, 2018

I've fixed this error in v2.0.2 :) Thanks for reporting!

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