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

Issue 12 - Print the list of registered routes #17

Closed
wants to merge 2 commits into from
Closed

Issue 12 - Print the list of registered routes #17

wants to merge 2 commits into from

Conversation

jadach1
Copy link

@jadach1 jadach1 commented Feb 22, 2019

Added method to print list of all registered routes with versions included.

Output is as follows:

**LIST OF STATIC AND DYNAMIC ROUTES

GET
{
/home/:b
/this/is/:dynamic/with/:two(\d+):params
/login/as/:role(admin|user|staff)
/this/is/:dynamic/and/*
/some/route { with version => 1.2.0 }
/some/route { with version => 1.2.3 }
/this/is/static
/this/is/static { with version => 1.2.0 }
/this/is/:dynamic
}

POST
{
/
/home/:A
/this/is/:dynamic
}**

@coveralls
Copy link

coveralls commented Feb 22, 2019

Pull Request Test Coverage Report for Build 89

  • 5 of 19 (26.32%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.3%) to 91.771%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/letsRoute.js 5 19 26.32%
Totals Coverage Status
Change from base Build 87: -2.3%
Covered Lines: 450
Relevant Lines: 482

💛 - Coveralls

src/letsRoute.js Outdated
});

// Save all the methods which we will use to itereate through to print the routes
listOfMethods[method] = {};
Copy link
Member

Choose a reason for hiding this comment

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

Instead, you can do,

listOfMethods[method][url]=[]
if(options.vetsion){
   listOfMethods[method][url].push(options.vetsion)
}

Above is just the code to explain my thought. You'll have to handle edge scenario.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the reply and suggestion. However, the "npm test" will fail whenever I reference the options.version property if it is undefined. So even with your suggestion above with the:

if(options.version){
listOfMethods[method][url].push(options.version)
}

Will cause a failure in the npm test runs when we are checking an undefined propery because not every object has a version implemented.

The following is the output error of the npm test

Failures:

  1. Anumargak events should throw error for unsupported events
    Message:
    Expected function to throw an exception with message 'Router: Unsupported event other', but it threw an exception with message 'Cannot read property 'version' of undefined'.
    Stack:
    Error: Expected function to throw an exception with message 'Router: Unsupported event other', but it threw an exception with message 'Cannot read property 'version' of undefined'.
    at
    at UserContext. (C:\Users\Jacob\OSD600\anumargak\tests\event_test.js:82:12)
    at

  2. Anumargak events should emit not-found and request event when the route is not registered
    Message:
    TypeError: Cannot read property 'version' of undefined
    Stack:
    at
    at Anumargak.on (C:\Users\Jacob\OSD600\anumargak\src\letsRoute.js:59:17)
    at UserContext. (C:\Users\Jacob\OSD600\anumargak\tests\event_test.js:53:16)
    at
    at runCallback (timers.js:705:18)
    at tryOnImmediate (timers.js:676:5)

  3. Anumargak events should emit found, request, and end event when the route is registered
    Message:
    TypeError: Cannot read property 'version' of undefined
    Stack:
    at
    at Anumargak.on (C:\Users\Jacob\OSD600\anumargak\src\letsRoute.js:59:17)
    at UserContext. (C:\Users\Jacob\OSD600\anumargak\tests\event_test.js:9:16)
    at
    at ontimeout (timers.js:436:11)
    at tryOnTimeout (timers.js:300:5)

91 specs, 3 failures
Finished in 1.618 seconds
Randomized with seed 87965 (jasmine --random=true --seed=87965)
npm ERR! Test failed. See above for more details.

If you have any ideas of how to get around this blocking issue that would be great.

Copy link
Member

Choose a reason for hiding this comment

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

:) you need to first check if the options variable is set or not.

Copy link
Author

Choose a reason for hiding this comment

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

I think you misunderstood me lol, but I apologize if I was not being clear. That is from my old PR, since then I have made other changes related to your suggestion. Primarily checking if the option variable is null or not. So I implemented this

if(options.version){
listOfMethods[method][url].push(options.version)
}

But it still causes the test case failures which I have output above.

This is strange because you have used the if(options.version) syntax in 2 other methods inside the app: checkIfRoutsPresent and getRouteHandlers

I believe the issue is derived from how the logic in the "./tests/event_test.js" is written, because there are no tests which reference the methods checkIfRoutsPresent and getRouteHandlers so the test files were written for the method on and it is returning something unexpected when I reference the if(options.version) syntax and is causing a failure.

I propose a few solutions:

  1. Rewrite the test case to expect an unknown value for the options.version property
  2. Print the list of routes and versions from the objects staticRoutes and dynamicRoutes
  3. Grab the routes from another function near the registration Anumargak.prototype.on method

If you have another solution proposition I would like to hear it. I hope I have made the situation a bit more clear and less confusing this time around.

Copy link
Author

Choose a reason for hiding this comment

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

I have another possible solution to avoid the failing tests. I have discovered that if I move the

if(options.version){
listOfMethods[method][url].push(options.version)
}

To the end of the .on method it will pass the test cases successfully. So another solution would be to move saving the list of routes to print to the end of the function as so:

Anumargak.prototype.on = function (method, url, options, fn, extraData) {

if (Array.isArray(url)) {
    for (var i = 0; i < url.length; i++) {
        this.on(method, url[i], options, fn, extraData);
    }
    return this;
}

if (typeof url === 'function') {
    this._onEvent(method, url);
    return this;
} else if (typeof options === 'function' || Array.isArray(options)) {
    extraData = fn;
    fn = options;
    options = {};
}

if (typeof method === "string") {
    if( method.toLocaleLowerCase() === 'all'){
        this.all(url, options, fn, extraData);
    }else{
        this._on(method, url, options, fn, extraData);
    }
} else if (Array.isArray(method)) {
    for (var i = 0; i < method.length; i++) {
        this._on(method[i], url, options, fn, extraData);
    }
} else {
    throw Error("Invalid method argument. String or array is expected.");
}
      // Save all the methods which we will use to itireate through to print the routes
      listOfMethods[method][url] = {};

     if(options.version){
          listOfMethods[method][url].push(options.version)
      }
return this;

}

There might be some unforeseen issues if the logic is written this way but I am not as familiar with this project as you are. If you have any input towards this solution, or any of the others in the previous comment it would be great to hear them.

Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not going through rest of the comment but replying this first.

This is strange because you have used the if(options.version) syntax in 2 other methods inside the app: checkIfRoutsPresent and getRouteHandlers

Check the Anumargak.prototype.on method carefully. I was checking the value of options and assigning with blank object. Hence, I need not to check it everytime in other methods.

Now you've added this code before any other code line in the same method;

    routesToPrint.push({
        method:  method,
        url:     url,
        version: options.version
});

Since options is optional and accessing any property of null or undefined in javascript is not allowed, you're getting this error. It is clearly understandable from the error description you attached.

Copy link
Member

@amitguptagwl amitguptagwl left a comment

Choose a reason for hiding this comment

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

please check my comment. Please also write unit tests.

@jadach1 jadach1 closed this by deleting the head repository Jun 1, 2023
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.

3 participants