#68 - Implement shorthand notation for middleware paths by bajtos · Pull Request #70 · strongloop/loopback-boot

bajtos · 2014-11-19T12:56:35Z

When the middleware name (path) is in the format {module}/{filename},
loopback-boot resolves the path by trying multiple locations and
using the first one that exists:

{module}/{filename} - the path as supplied,
-> e.g. loopback/rest
{module}/middleware/{filename}
-> e.g. loopback/middleware/rest
{module}/server/middleware/{filename}
-> e.g. loopback/server/middleware/rest

Values in any other format will bypass this resolution algorithm and
they will be used in the original form:

a full path in a module: loopback/server/middleware/rest
a relative path: ./middleware/custom, ./custom, ../logger
an absolute path: /usr/local/lib/node_modules/compression

Close #68

/to @ritch please review
/cc @raymondfeng

raymondfeng · 2014-11-19T16:30:29Z

Two suggestions:

Can we use # for shorthand path, for example, loopback#rest and leave / for require paths?
I would like to support the middleware exported in the main script of a module, for example,

loopback/index.js

exports.rest = function(options) {
...
}

loopback#rest should check the presence of require('loopback').rest.

bajtos · 2014-11-19T17:28:47Z

Can we use # for shorthand path, for example, loopback#rest and leave / for require paths?

FWIW, # is a valid path character, although it is not allowed in npm package names.

Why is # better than /? And why not some other character, e.g. loopback%rest?

I would like to support the middleware exported in the main script of a module, for example,

Please open a new GH issue to track this requirement.

raymondfeng · 2014-11-19T17:46:43Z

I'm following the url pattern. # represents a fragment (http://tools.ietf.org/html/rfc3986#section-3.5) and it is a good fit for our usage IMO.

bajtos · 2014-11-19T18:15:18Z

I'm following the url pattern. # represents a fragment (http://tools.ietf.org/html/rfc3986#section-3.5) and it is a good fit for our usage IMO.

Using the same logic, loopback/server/middleware/rest should have been written as loopback#server/middleware/rest, right?

We can use # for middleware exported by the main scripts of a module to distinguish it from path-based ids.

bajtos · 2014-11-19T18:54:34Z

@raymondfeng Can we agree that # should not be used for the case where loopback/rest is resolved to loopback/server/middleware/rest.js, land this PR as it is now, and move the discussion about how to access require('loopback').favicon to a new issue/pull request?

raymondfeng · 2014-11-19T19:01:55Z

@bajtos I would prefer not to add resolution magic if the path is for require. The resolution will only kick in if there is a # fragment. For example:

loopback/rest ==> node_modules/loopback/rest.js
loopback/server/middleware/rest ==> node_modules/loopback/server/middleware/rest.js
loopback#rest ==> trying the following in order:
- require('loopback').rest
- node_modules/loopback/rest i.e., require('loopback/rest')
- node_modules/loopback/middleware/rest
- node_modules/loopback/server/middleware/rest

bajtos · 2014-11-19T19:08:05Z

I can see the value in your proposal too. @ritch, could you chime in and let us know your opinion too? If you agree with @raymondfeng, then I will rework my pull request to implement his proposal.

I am not sure if it is a good thing to use # instead of / and support require('loopback').rest, but since I don't have any convincing arguments, I'd rather implement it as requested and move on.

bajtos · 2014-11-19T19:08:49Z

Actually, if this is a blocker for you @raymondfeng, then it would be best if you could take this over, as I won't have time to finish this until Monday (most likely).

bajtos · 2014-11-19T19:11:44Z

One last though: later, when we enhance loopback-workspace, Arc and loopback-generator to offer a configuration UI based on metadata provided by middleware in some JSON file, we won't be able to infer the config properties for the middleware referenced via require('loopback').rest.

raymondfeng · 2014-11-19T19:14:27Z

I can rework the PR to move forward.

ritch · 2014-11-19T21:17:34Z

IMO # style should be documented as "short form".

// long form (allows you to explicitly pass to require)
// detected when a `#` is not present
"loopback/middleware/rest"

// short form (opts into our middleware search algorithm)
"loopback#rest"

The short form precedence should be:

module/server/middleware/fragment
module/middleware/fragment
module.fragment (no way to know this is even a middleware, use a middleware defined in a directory named middleware first)

Note: this excludes support for loopback#rest => loopback/rest

raymondfeng · 2014-11-19T23:38:32Z

@ritch I think module.fragment needs take precedence as there is no other format to allow that. If a developer needs to choose module/server/middleware/fragment over module.fragment, he/she can use the path module/server/middleware/fragment' instead ofmodule#fragment`.

raymondfeng · 2014-11-20T00:12:56Z

The code has been updated to reflect:

// long form (allows you to explicitly pass to require)
// detected when a `#` is not present
"loopback/middleware/rest"

// short form (opts into our middleware search algorithm)
"loopback#rest"

The short form precedence should be:

module.fragment (no way to know this is even a middleware, use a middleware defined in a directory named middleware first)
module/server/middleware/fragment
module/middleware/fragment

Note: this excludes support for loopback#rest => loopback/rest

raymondfeng · 2014-11-20T00:32:09Z

FYI: I was able to bring up strong-gateway using https://github.com/strongloop/strong-gateway/blob/feature/externalize-middleware/server/middleware.json.

bajtos · 2014-11-20T08:40:39Z

One more thought to consider: how are we going to handle / in the shorthand form, e.g. loopback#server/rest and loopback/server#rest? Should we reject such ids?

bajtos · 2014-11-20T08:42:35Z

Please don't use /**, that's indicating a jsdoc comment. Use plain comment /* instead.

bajtos · 2014-11-20T09:16:47Z

@raymondfeng please add two more tests:

in test/executor.test.js, add a pure unit-test exercising the new instruction format built from foo#bar
integration test - modify test/fixtures/simple-app to load a middleware using foo#bar notation, add a test to verify the middleware was loaded as expected.

Is there any reason why we are not supporting # for relative paths too, e.g. ../middleware#name?

raymondfeng · 2014-11-20T18:29:36Z

@bajtos I fixed the code to honor # for all paths and addressed your comments except the executor test case. I was trying to add a node module for simple-app but it doesn't help. The test case is executed in the context of the loopback-boot and require doesn't even search simple-app/node_modules. How can we force that?

bajtos · 2014-11-20T22:18:45Z

The test case is executed in the context of the loopback-boot and require doesn't even search simple-app/node_modules. How can we force that?

Hmm, that's a good point. This is a problem similar to strongloop/loopback#833. A possible solution is to use resolveSourceDir instead of resolve.require, plus throw an exception if the path does not exist (resolveSourceDir returns undefined).

bajtos · 2014-11-20T22:20:07Z

Commented-out code - please remove it. If you feel it should serve as a todo, then annotate it with a todo comment explaining that intent.

raymondfeng · 2014-11-20T23:17:21Z

Comments addressed.

bajtos · 2014-11-21T19:59:42Z

Why is it necessary to trim the value?

No harm to trim it, for example, " ./lib/x": {} will be supported.

We don't trim any other values anywhere else in loopback. I prefer to be consistent and either trim all values or none. Since this is the first place, I am proposing to remove this line and implement trimming in a new pull request if you are convinced it is a good feature (I am personally not).

bajtos · 2014-11-21T20:03:53Z

@raymondfeng What's your opinion on the following? Do you feel like fixing it in this PR, or is it out of scope?

The test case is executed in the context of the loopback-boot and require doesn't even search simple-app/node_modules. How can we force that?

Hmm, that's a good point. This is a problem similar to strongloop/loopback#833. A possible solution is to use resolveSourceDir instead of resolve.require, plus throw an exception if the path does not exist (resolveSourceDir returns undefined).

raymondfeng · 2014-11-21T20:53:09Z

My preference is merge the PR and create an issue to add the ability to test execution against an embedded sandbox app.

bajtos · 2014-11-21T20:54:04Z

My preference is merge the PR and create an issue to add the ability to test execution against an embedded sandbox app.

Sounds good, please create the issue before we merge the PR.

bajtos · 2014-11-21T21:01:12Z

The PR is still missing the two tests I mentioned in my earlier comment:

in test/executor.test.js, add a pure unit-test exercising the new instruction format built from foo#bar
integration test - modify test/fixtures/simple-app to load a middleware using foo#bar notation, add a test to verify the middleware was loaded as expected.

raymondfeng · 2014-11-21T21:09:10Z

I added the tests and keep it disabled for now until #73 is fixed. LGTY now?

bajtos · 2014-11-21T21:29:49Z

I am missing check that loopback#static was configured.

I strongly dislike keeping around commented-out code (my-module#myMiddleware), just remove it and add it back as part of #73.

As for testing my-module#myMiddleware syntax, you can surely use loopback#errorHandler or loopback#favicon, as that should already work now, even without #73? You will need a different HTTP request to ensure the middleware was executed, therefore it is better to move this stuff to a different test (a new it() block).

raymondfeng · 2014-11-21T21:58:17Z

Enhanced the tests and added it.skip() for one test yet to be supported.

bajtos · 2014-11-24T14:19:09Z

What is the point of this block? I does not seem to be used by the test code below.

When the middleware name (path) is in the format {module}#{filename}, loopback-boot resolves the path by trying multiple locations and using the first one that exists: - {module} and check the {filename} property of the exports -> e.g. loopback.rest - {module}/server/middleware/{filename} -> e.g. loopback/server/middleware/rest - {module}/middleware/{filename} -> e.g. loopback/middleware/rest Values in any other format will bypass this resolution algorithm and they will be used in the original form: - a full path in a module: loopback/server/middleware/rest - a relative path: ./middleware/custom, ./custom, ../logger - an absolute path: /usr/local/lib/node_modules/compression

bajtos · 2014-11-25T10:43:40Z

I have addressed the comments myself and amended the commit.

bajtos · 2014-11-25T10:48:51Z

@slnode test please

#68 - Implement shorthand notation for middleware paths

bajtos added the #review label Nov 19, 2014

bajtos assigned ritch Nov 19, 2014

bajtos added this to the #Epic: Components milestone Nov 19, 2014

bajtos changed the title ~~Implement shorthand notation for middleware paths~~ #68 - Implement shorthand notation for middleware paths Nov 19, 2014

raymondfeng mentioned this pull request Nov 19, 2014

Expose more loopback middleware for require strongloop/loopback#834

Merged

raymondfeng force-pushed the feature/short-middleware-paths branch from 2806b4d to 6b86b17 Compare November 19, 2014 23:56

bajtos reviewed Nov 20, 2014
View reviewed changes

raymondfeng force-pushed the feature/short-middleware-paths branch from 6b86b17 to 7a40224 Compare November 20, 2014 17:26

bajtos reviewed Nov 20, 2014
View reviewed changes

bajtos reviewed Nov 21, 2014
View reviewed changes

raymondfeng force-pushed the feature/short-middleware-paths branch from fde3970 to cb07de7 Compare November 21, 2014 20:51

raymondfeng force-pushed the feature/short-middleware-paths branch from cb07de7 to f781937 Compare November 21, 2014 20:55

raymondfeng mentioned this pull request Nov 21, 2014

Resolve module paths as relative to appRootDir instead of loopback-boot/lib #73

Closed

raymondfeng force-pushed the feature/short-middleware-paths branch from f781937 to d3a5d58 Compare November 21, 2014 21:08

bajtos reviewed Nov 21, 2014
View reviewed changes

raymondfeng force-pushed the feature/short-middleware-paths branch from d3a5d58 to bed6ba7 Compare November 21, 2014 21:56

bajtos reviewed Nov 24, 2014
View reviewed changes

This was referenced Nov 24, 2014

Document middleware phases strongloop/loopback#863

Closed

Middleware Mounting #44

Closed

bajtos force-pushed the feature/short-middleware-paths branch from bed6ba7 to 2f72006 Compare November 25, 2014 10:43

bajtos assigned bajtos and unassigned ritch Nov 25, 2014

bajtos added a commit that referenced this pull request Nov 25, 2014

Merge pull request #70 from strongloop/feature/short-middleware-paths

6de571f

#68 - Implement shorthand notation for middleware paths

bajtos merged commit 6de571f into master Nov 25, 2014

bajtos removed the #review label Nov 25, 2014

bajtos deleted the feature/short-middleware-paths branch November 25, 2014 10:51

Uh oh!

Conversation

bajtos commented Nov 19, 2014

Uh oh!

raymondfeng commented Nov 19, 2014

Uh oh!

bajtos commented Nov 19, 2014

Uh oh!

raymondfeng commented Nov 19, 2014

Uh oh!

bajtos commented Nov 19, 2014

Uh oh!

bajtos commented Nov 19, 2014

Uh oh!

raymondfeng commented Nov 19, 2014

Uh oh!

bajtos commented Nov 19, 2014

Uh oh!

bajtos commented Nov 19, 2014

Uh oh!

bajtos commented Nov 19, 2014

Uh oh!

raymondfeng commented Nov 19, 2014

Uh oh!

ritch commented Nov 19, 2014

Uh oh!

raymondfeng commented Nov 19, 2014

Uh oh!

raymondfeng commented Nov 20, 2014

Uh oh!

raymondfeng commented Nov 20, 2014

Uh oh!

bajtos commented Nov 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Nov 20, 2014

Uh oh!

raymondfeng commented Nov 20, 2014

Uh oh!

bajtos commented Nov 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Nov 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Nov 21, 2014

Uh oh!

raymondfeng commented Nov 21, 2014

Uh oh!

bajtos commented Nov 21, 2014

Uh oh!

bajtos commented Nov 21, 2014

Uh oh!

raymondfeng commented Nov 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Nov 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Nov 25, 2014

Uh oh!

bajtos commented Nov 25, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development