balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.84k stars 1.95k forks source link

Sails 1.0 custom static sources #5306

Open cope opened 5 years ago

cope commented 5 years ago

Node version: 8.15.0 Sails version (sails): 1.1.0 ORM hook version (sails-hook-orm): 2.1.1 Sockets hook version (sails-hook-sockets): 1.5.5 Organics hook version (sails-hook-organics): 0.15.2 Grunt hook version (sails-hook-grunt): 3.1.0 Uploads hook version (sails-hook-uploads): - DB adapter & version (e.g. sails-mysql@5.55.5): sails-mongo 1.0.1 Skipper adapter & version (e.g. skipper-s3@5.55.5): -


Prior to Sails 1.0 one could use the customMiddleware to add static sources:

customMiddleware: function (app) {
    app.use('/ui', express.static(process.cwd() + '/ui'));
    app.use('/docs', express.static(process.cwd() + '/docs'));
    app.use('/node_modules', express.static(process.cwd() + '/node_modules'));
},

But now in Sails 1.0 the customMiddleware is deprecated:

debug: Warning: use of `customMiddleware` is deprecated in Sails 1.0.
debug: Instead, use an Express 4-compatible middleware (res, res, next) function.
debug: See http://sailsjs.com/docs/upgrading/to-v-1-0#?express-4 for more info.

And the referenced link does not say how to replace app.use('/ui', express.static(process.cwd() + '/ui')); anywhere.

I've tried so many things, including using serve-static and also trying to set up custom routes, but nothing works.

Is it really possible that in Sails 1.0 we cannot set up custom static sources?

Here is the stackoverflow link .

sailsbot commented 5 years ago

@cope Thanks for posting! We'll take a look as soon as possible.

In the mean time, there are a few ways you can help speed things along:

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

raqem commented 5 years ago

Hi @cope could you please give us insight into your use case? Why do you need to separate your files into multiple folders? I did talk to @mikermcneil about your issue and he suggested you consider some of these options he has offered to others. The one he suggested the strongest is mounting them as routes. You can try it like this:

'GET /ui': require('serve-static')(require('path').join(process.cwd(), 'ui/'), {
  maxAge: process.env.NODE_ENV !== 'production' ? 1 : 31557600000
}),

To be able to use this you'll have to install serve-static npm install serve-static@1.13.1 --save-exact

cope commented 5 years ago

Hi @raqem, one example would be node_modules, since we have switched from bower+npm to just yarn, we now have both server and client side packages together, so we need a way to load some of them in our page, thus a static ref.

I will try your suggestion right now, although I have tried something like that yesterday already, only without the GET. Let's see...

cope commented 5 years ago

Nope, adding a route did not work :(

const path = require('path');
const serveStatic = require('serve-static');

module.exports.routes = {

    'GET /ui': serveStatic(path.join(process.cwd(), 'ui/'), {maxAge: 31557600000}),

GET https://localhost:3100/ui/main/Login.js net::ERR_ABORTED 404 (Not Found)

cope commented 5 years ago

I've also tried adding my own middleware:

        ui: (function () {
            let uiMiddleware = serveStatic(process.cwd() + '/ui', {maxAge: 31557600000});
            return uiMiddleware;
        })(),

Same result...

rachaelshaw commented 5 years ago

@cope just took a closer look at the docs for serve-static and realized that req.url gets combined with the root directory when determining which file to serve. So, in the example above, it would actually be looking for /ui/ui/foo. I tried adding the following to my routes file in a test app and it worked for me:

'GET /*': {
    skipAssets: false,
    fn: [
      require('serve-static')(require('path').resolve(__dirname, '../', 'assets2/'), {
        maxAge: process.env.NODE_ENV !== 'production' ? 1 : 31557600000,
      }),
      // require('serve-static')(require('path').resolve(__dirname, '../', 'assets3/'), {
      //   maxAge: process.env.NODE_ENV !== 'production' ? 1 : 31557600000,
      // }),
      // require('serve-static')(require('path').resolve(__dirname, '../', 'assets4/'), {
      //   maxAge: process.env.NODE_ENV !== 'production' ? 1 : 31557600000,
      // }),
      //…
    ]
},

(One thing to keep in mind is that if any of the directories from that array have files with the same name, the one listed first will take precedence - /assets2/ wouldn't be included in the route to the file.)

Hope this helps!

cope commented 5 years ago

Nope, same error GET https://localhost:3100/ui/main/Login.js net::ERR_ABORTED 404 (Not Found)

I tried:

module.exports.routes = {

'GET /*': {
    skipAssets: false,
    fn: [
        require('serve-static')(require('path').resolve(__dirname, '../', 'ui/'), {
            maxAge: process.env.NODE_ENV !== 'production' ? 1 : 31557600000,
        })
    ]
},

I tried skipAssets: true,, I tried 'GET /ui': {... nothing works :(

The only thing that works is

    customMiddleware: function (app) {
        app.use('/ui', require('express').static(process.cwd() + '/ui'));

And yet customMiddleware is deprecated and is going away...

raqem commented 5 years ago

Hi @cope based on what @rachaelshaw said I think you need to remove ui from your GET request.

GET https://localhost:3100/main/Login.js
cope commented 5 years ago

I don't want to lose the ui from my request, I want my static files to be where I want them to be and to be requested via a path I chose.

The ui is just one example. My project is huge, I have no intention of doing this much work for something that should be a rather standard functionality... It is a standard functionality in express, is it not? So why is sails actually blocking it now in 1.0?

raqem commented 5 years ago

Hi @cope after thorough testing on our end we were able to set custom routes with the example @rachaelshaw provided. And with a little bit of editing to that code you should be able to set a route for all 4 of the examples you provided. Hope that works and please let us know how it goes.

asadakbar commented 5 years ago

@raqem Can you please give an example using the folder names that @cope said? Or take a look at my example below as its very similar in need. I need to remove etag generation and am trying to mount a route for serving up the minified asset files in production so that I can pass options to serve-static. I've got this so far

  'get /min/*': { 
    skipAssets: false,
    fn: [
      serveStatic(path.join(__dirname, '/min/'), {
        maxAge: process.env.NODE_ENV !== 'production' ? 1 : 30000,
        etag: false
      }),
    ]
  }

But it doesn't seem to be catching the requests to the /min folder that compiled assets are being served from in production. Do you know what I'm doing wrong?

whichking commented 5 years ago

Hey, @asadakbar! Could you try removing the leading forward slash and using resolve instead of join? Like so:

  'get min/*': { 
    skipAssets: false,
    fn: [
      serveStatic(require('path').resolve(__dirname, 'min/'), {
        maxAge: process.env.NODE_ENV !== 'production' ? 1 : 30000,
        etag: false
      }),
    ]
  }
asadakbar commented 5 years ago

@madisonhicks Thanks for the help. The asset files are found but etags still seem to be there. I'm not sure if thats because

  1. This new route is not being used to serve the assets and the old route that I'm assumed is generated somewhere else is being used still. Maybe the new route needs to be last in the list of routes?

or

  1. The new route is being used but I'm not turning off etags correctly.

There are other people with the same issue so I've been hoping to find a solution for myself and them. https://github.com/balderdashy/sails/issues/3750 https://stackoverflow.com/questions/37127173/remove-etag-from-sailsjs#comment61819018_37127173

Edit: I'm using sails v0.12.14.

asadakbar commented 5 years ago

tl;dr I got it to work with this

  'get /min/*': { 
    skipAssets: false,
    fn: [
      require('serve-static')('.tmp/public', {
        maxAge: process.env.NODE_ENV !== 'production' ? 0 : 31557600000,
        etag: false
      }),
    ]
  }

and readers can see below for how to remove etags or pass any options for all asset files, not just those in /min


For the deeper dive, __dirname refers to the current working directory. Since the routes.js file exists in /config, the url I would be passing to serveStatic would be /config/min, which is not where the asset files are, thus why the code currently isn't working.

I changed what I had to this and I was able to reach the asset files:

  'get /min/*': { 
    skipAssets: false,
    fn: [
      require('serve-static')('.tmp/public', {
        maxAge: process.env.NODE_ENV !== 'production' ? 0 : 31557600000,
        etag: false
      }),
    ]
  }

Since the asset files are being served out of .tmp/public as defined here

https://github.com/balderdashy/sails/blob/c7900af9864a10bde3fdc83097d99b82cddc713a/lib/hooks/http/index.js#L39-L41 serveStatic can now find the asset files and pass in the new options.

Whatever directory is being passed into serveStatic is set as a root path https://github.com/expressjs/serve-static/blob/master/index.js#L65 that is passed to nodejs#send as an option along with the url path https://github.com/expressjs/serve-static/blob/master/index.js#L88-L96.

Send adds the pathname to the end of the root directory we initially passed to serveStatic and returns our asset file(https://github.com/pillarjs/send#serve-all-files-from-a-directory).

For example, a request GET /foo.txt will send back /www/public/foo.txt.

@cope I would fix the directory structure you are passing to serveStatic and I think it will start working.

For my purposes I've gotten etagging of asset files served out of /min to be removed. I needed this to happen because I am running two instances of the app and the production.css and production.js files kept being redownloaded depending on which instance of the app the user hit on their request. This was due to the etags being different on each instance even though the files were the same.

I was also able to figure out how to remove etags from all asset files, which is the issue others are also having. I don't want to because I still need to add cache stamps to the image and font names so their caches can be busted, so etags are good enough for now. This is how those that need to do that can do it:

    www: (function() {
      var flatFileMiddleware = require('serve-static')('.tmp/public', {
        maxAge: 31557600000,
        etag: false
      });

      return flatFileMiddleware;
    })(),

should be added to the /config/http.js file as an override to the default implementation of the www middleware. The maxAge must be defined here and overrides any age that is set by the cache key defined as an option in this file.

For the long term, I think it makes sense to give users the ability to override any of the options being passed to serveStatic by the default implementation of the www middleware. Currently its only cache (https://github.com/balderdashy/sails/blob/master/lib/hooks/http/get-configured-http-middleware-fns.js#L54-L60). I can work on a pr if there is interest. Also would be good to add something to the documentation on assets (https://sailsjs.com/documentation/concepts/assets mentions the cache option).

Apologies for turning this into a novel, I really wanted to get this problem solved! Hope it helps.

johnabrams7 commented 5 years ago

@asadakbar - Fantastic! Thanks for the detailed solution! 🕺 @cope Give this a go 😎

cope commented 5 years ago

@johnabrams7 we punted on 1.0 due to the Oracle driver issues, in addition to this problem.

The plan is to go back at a later date, once the Oracle driver is working with 1.0 and try again.