Add app.createRouter function #3623

czaarek99 · 2018-04-16T14:47:01Z

Implement PR #3622 but against 4.x master branch instead

wesleytodd · 2018-04-16T16:27:57Z

lib/application.js

+ * @public
+ */
+app.createRouter = function (options) {
+  options = options || {}


Since this modifies the options below, can you make it not modify the incoming options object?

var opts = options || {}; // ... opts.strict = this.enabled('strict routing')

It still looks the same. Did you commit the fix?

@dougwilson Yeah, it just doesn't seem like it. Look at the whole file and you'll see its fine.

I did and it still says options = options || {} can you write here what you see this line as?

@dougwilson Um yes... But it's not modifying the original object anymore because the code changed below.

Well I mean, technically it is modifying it but only if it returns a falsy value in which case the user didn't actually pass anything into the function and in which case it doesn't matter.

The argument is still being redefined. Just use the code Wes suggested. Do not reassign arguments.

It does matter due to how v8 optimized and compiles functions.

@dougwilson Oh I didn't realize. I fixed it now.

czaarek99 · 2018-04-16T17:27:45Z

Seems like I broke the tests with my latest commit. I'll look into it.

czaarek99 · 2018-04-16T17:38:59Z

Fixed.

dougwilson

Awesome, looking good 👍 I'll fix the coverage reporting on the PR as it should be failing because one of your lines is not covered by a test. You can run npm run test-cov to generate a coverage report locally until I get the CI upload fixed.

dougwilson · 2018-04-16T18:13:01Z

lib/application.js

+    caseSensitive: this.enabled('case sensitive routing')
+  }
+
+  if (options.hasOwnProperty('caseSensitive')) {


Don't use hasOwnProperty to access the key. Just follow the existing patterns so we don't break people who configure with plain objects

dougwilson · 2018-04-16T18:13:19Z

lib/application.js

+    opts.caseSensitive = options.caseSensitive
+  }
+
+  if (options.hasOwnProperty('strict')) {


Same hop concern here.

czaarek99 · 2018-04-16T18:53:25Z

@dougwilson The CI test should be 100% now and I stopped using hasOwnProperty

dougwilson · 2018-04-16T18:56:29Z

test/app.createRouter.js

+    })
+
+    assert.equal(router.strict, false, 'options param strict did not override app.settings')
+    assert.equal(router.caseSensitive, false, 'options param caseSensitive did not override app.settings')


How do you know if this false is coming from the app settings or from the override?

It's true by accident, I'll get it fixed.

dougwilson · 2018-04-16T19:00:48Z

lib/application.js

+ * and allows them to be overriden by passing in an options
+ * object
+ *
+ * @param {Object} options


We had a PR about this right now, so we should make sure we land it right this time :) The options argument needs to be optional in the JSDoc here since it's optional in the code 👍

dougwilson · 2018-04-16T19:05:58Z

lib/application.js

+ */
+app.createRouter = function (options) {
+  var opts = {
+    strict: this.enabled('case sensitive routing'),


I think the case sensitive routing is a typo on this line. Let's fix that and add a test that will actually catch that mistake in the future 👍

After staring at a screen for this many hours eventually you go blind 😆

That's why we review code 😄 happens to everyone

czaarek99 · 2018-04-16T19:10:22Z

Fixed the optional argument, tests and me being blind (hopefully).

wesleytodd · 2018-04-16T19:51:47Z

lib/application.js

+
+  return Router(opts)
+}
+


This is mostly just a style thing, so if there is strong opposition feel free to ignore me, but I find that reducing the code required to achieve the same result often reduces maintenance and understanding costs in the future. For example, this could be written much more succinctly as follows:

app.createRouter = function (options) { var opts = options || {} opts.strict = options.strict !== undefined ? options.strict : this.enabled('strict routing') opts.caseSensitive = options.caseSensitive !== undefined ? options.caseSensitive : this.enabled('case sensitive routing') return Router(opts) }

Sometimes more lines of code is a good thing for clarity, but in this case, there are a bunch of lines which can be achieved with one usage of a ternary. This also has the upside of not unnecessarily calling this.enabled if you pass the option.

I like that we don't have to call this.enabled if the options are passed in, that's good. But to me this looks like a jumbled mess. It takes multiple seconds to decode what it actually means while undefined checks are really easy to make sense of imo.

Like I said, mostly a style thing :), but maybe you can find a middle ground without the unnecessary calls?

Also, technically it is just the same undefined checks you had but in a single line. Maybe other people like the more verbose syntax with the same result, I don't know and it probably shouldn't be the focus of any more energy.

app.createRouter returns a router which inherits settings from app.settings unless they're overriden in the options param

czaarek99 · 2018-04-17T17:50:27Z

@wesleytodd The new code skips uneccessary this.enabled calls but uses the more verbose if syntax. Seems like the best solution to me 😸

wesleytodd · 2018-04-17T18:01:51Z

Awesome, this is much more clean and clear. Thanks for the good work!

czaarek99 force-pushed the app-create-router-4.x branch from bda0c07 to d9ea79d Compare April 16, 2018 14:50

wesleytodd reviewed Apr 16, 2018

View reviewed changes

czaarek99 force-pushed the app-create-router-4.x branch from d9ea79d to 1a48254 Compare April 16, 2018 17:05

czaarek99 force-pushed the app-create-router-4.x branch from 1a48254 to 0f645b6 Compare April 16, 2018 17:38

dougwilson suggested changes Apr 16, 2018

View reviewed changes

czaarek99 force-pushed the app-create-router-4.x branch from 4a4a681 to 0b475bb Compare April 16, 2018 18:51

dougwilson suggested changes Apr 16, 2018

View reviewed changes

dougwilson added enhancement pr labels Apr 16, 2018

czaarek99 force-pushed the app-create-router-4.x branch from 0b475bb to f9296aa Compare April 16, 2018 19:00

dougwilson suggested changes Apr 16, 2018

View reviewed changes

czaarek99 force-pushed the app-create-router-4.x branch from f9296aa to b207a68 Compare April 16, 2018 19:05

dougwilson suggested changes Apr 16, 2018

View reviewed changes

czaarek99 force-pushed the app-create-router-4.x branch from b207a68 to 2d62933 Compare April 16, 2018 19:07

wesleytodd reviewed Apr 16, 2018

View reviewed changes

Add app.createRouter function

447e756

app.createRouter returns a router which inherits settings from app.settings unless they're overriden in the options param

czaarek99 force-pushed the app-create-router-4.x branch from 2d62933 to 447e756 Compare April 17, 2018 17:49

wesleytodd approved these changes Apr 17, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

dougwilson added this to the 4.17 milestone Sep 24, 2018

dougwilson mentioned this pull request Oct 27, 2018

Release 4.17 #3778

Merged

23 tasks

dougwilson removed this from the 4.17 milestone May 10, 2019

3imed-jaberi approved these changes Aug 17, 2019

View reviewed changes

czaarek99 closed this Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add app.createRouter function #3623

Add app.createRouter function #3623

czaarek99 commented Apr 16, 2018

wesleytodd Apr 16, 2018

czaarek99 Apr 16, 2018

dougwilson Apr 16, 2018

czaarek99 Apr 16, 2018

dougwilson Apr 16, 2018

czaarek99 Apr 16, 2018

czaarek99 Apr 16, 2018 •

edited

Loading

dougwilson Apr 16, 2018

dougwilson Apr 16, 2018

czaarek99 Apr 16, 2018

czaarek99 commented Apr 16, 2018

czaarek99 commented Apr 16, 2018

dougwilson left a comment

dougwilson Apr 16, 2018

dougwilson Apr 16, 2018

czaarek99 commented Apr 16, 2018

dougwilson Apr 16, 2018

czaarek99 Apr 16, 2018 •

edited

Loading

dougwilson Apr 16, 2018

dougwilson Apr 16, 2018

czaarek99 Apr 16, 2018 •

edited

Loading

dougwilson Apr 16, 2018

czaarek99 commented Apr 16, 2018 •

edited

Loading

wesleytodd Apr 16, 2018

czaarek99 Apr 16, 2018

wesleytodd Apr 16, 2018

czaarek99 commented Apr 17, 2018 •

edited

Loading

wesleytodd commented Apr 17, 2018

This comment has been minimized.

This comment has been minimized.

Add app.createRouter function #3623

Add app.createRouter function #3623

Conversation

czaarek99 commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czaarek99 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czaarek99 commented Apr 16, 2018

czaarek99 commented Apr 16, 2018

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czaarek99 commented Apr 16, 2018

Choose a reason for hiding this comment

czaarek99 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czaarek99 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czaarek99 commented Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czaarek99 commented Apr 17, 2018 • edited Loading

wesleytodd commented Apr 17, 2018

This comment has been minimized.

This comment has been minimized.

czaarek99 Apr 16, 2018 •

edited

Loading

czaarek99 Apr 16, 2018 •

edited

Loading

czaarek99 Apr 16, 2018 •

edited

Loading

czaarek99 commented Apr 16, 2018 •

edited

Loading

czaarek99 commented Apr 17, 2018 •

edited

Loading