Skip to content

Conversation

@jacsamg
Copy link
Contributor

@jacsamg jacsamg commented Jun 14, 2018

Hi! 🙂

I added a new option that allows to use the integrated middleware through a specific route in express.

In the issue #15 had proposed a solution, but after reviewing the code calmly, I realized that the problem was only in the method that returns the versioned path: getVersionedPath.

It happens because when adding a route for the middleware, the method getVersionedPath has no knowledge of it, and continues to return the direct path.

With the new option it is possible to specify that the middleware will pass through a route.

The new option is used in the getVersionedPath method. In this way, it returns the path correctly even when there is a specific route for the middleware.

Example of use:

var path = require('path');
var options = { mwRoute: '/assets' }
var staticify = require('staticify')(path.join(__dirname, '/public'), options);

app.use('/assets', staticify.middleware);
app.locals.assets = staticify.getVersionedPath;

I await your comments. 👍

Fixes #15

jacsamg added 2 commits June 13, 2018 20:11
A new option was added that allows to use a specific route
with the middleware provided by staticify.

This option is used by the getVersionedPath method.

The specified route for the middleware is added before the
versioned path. When a specific route is not used, the method
continues to function normally.

Ref issue #15
The description and example of the new specific route option
was added in the README file.
@XhmikosR
Copy link
Collaborator

@tojacob: can you add tests?

@jacsamg
Copy link
Contributor Author

jacsamg commented Jun 14, 2018

@XhmikosR:
Currently I do not have much experience with the test libraries.
But I added the test for the new mwRoute option. I think it covers what is necessary in a test.

@rakeshpai
Copy link
Member

LGTM! Thanks for the PR, @tojacob.

I know this is bikeshedding, but is mwRoute the best name? Could we call it prefix or 'pathPrefix' or something along those lines? Open to suggestions.

This was referenced Jun 15, 2018
@XhmikosR
Copy link
Collaborator

Note that this should be squashed into one commit when merging.

@jacsamg
Copy link
Contributor Author

jacsamg commented Jun 15, 2018

@rakeshpai: It's true, I have serious problems with names.
It seems to me that pathPrefix looks great, it is even self-explanatory, because it is used with the getVersionedPath method. I'll change it so that it's ready.

@XhmikosR: Okay!

@XhmikosR XhmikosR merged commit 38e2e01 into errorception:master Jun 15, 2018
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