/console/console.js 404 when using prefix

[panlina]

Hi,
When I set prefix to /cloudcmd and press the console button, devtool shows a 404 for /cloudcmd/console/console.js.
The cause is that cloudcmd is prepending prefix to "/console" as prefix for console-io:
https://github.com/coderaiser/cloudcmd/blob/v11.5.3/server/cloudcmd.js#L143-L146
When it goes to console-io,
https://github.com/cloudcmd/console-io/blob/v9.0.0/server/index.js#L103
it will not work as it seems, because req.url will be "/console/console.js" instead of "/cloudcmd/console/console.js" due to the mounting feature of express. I tried changing prefix + '/console' to '/console' and it works.
I didn't read further in console-io, so I'm not sure if it'll break things in console-io. Can you take a look?

[coderaiser]

I do not know why you receive 404 when you set prefix /cloudcmd, but console has to kind of prefixes:

• http middleware prefix which is set in cloudcmd.js#L168-L171
• web sockets prefix which is set in cloudcmd.js#L143-L146

When it goes to console-io,
https://github.com/cloudcmd/console-io/blob/v9.0.0/server/index.js#L103
it will not work as it seems, because req.url will be "/console/console.js" instead of "/cloudcmd/console/console.js" due to the mounting feature of express

I can not reproduce this, in debugger I see such result:

Could you please tell me what version of node and express are you use?

[panlina]

You didn't reproduce it because you're running it as a standalone app, by executing cloudcmd --prefix /cloudcmd, or using it as a middleware at the root path.
However, as I mentioned above, when you use the mounting feature of express with cloudcmd, this issue comes in, because express strips the baseUrl for you.
I highly recommend making cloudcmd work with mounting if you offer its functionality as a middleware, because it's a very common pattern in express. With it you allow the middleware to work on any path without telling it the prefix.
To repro the issue, use the following code:

app.use('/cloudcmd', cloudcmd({
	socket,
	config: { prefix: "/cloudcmd" }
}));

Currently it only works by:

app.use('/', cloudcmd({	//or just omit the 1st argument
	socket,
	config: { prefix: "/cloudcmd" }
}));

I recommend it to be:

app.use('/cloudcmd', cloudcmd({	//or just omit the 1st argument
	socket,
	config: { /* no prefix is needed */ }
}));

It not only makes it easier to use, but also easier to implement, I guess, because you only deal with logical paths within your middleware. You can eliminate most of the prefix tricks in server side source code down the stack(console-io, etc.) I believe.
I appreciate it if you can consider my advice.

[coderaiser]

I understand what are you talking about, would be great to see it as a pull request :). And would be great to understand how to test it as well :)

[panlina]

I'm willing to contribute. I'll try when I have free time. 😄

[coderaiser]

OK, there is a couple things that should be considered.

When you do not set prefix directly client side of Cloud Commander doesn't know the place to lazy load data from. We should determine how to get express mount point you going to use to tell client side about.

In this case:

app.use('/cloudcmd', cloudcmd({
    socket,
    config: {
        prefix: ''
    }
}));

client/load-module which is used to load View, Edit, Console, Terminal, Config and Contact should know the mount point somehow.

When you use:

app.use('/cloudcmd', cloudcmd({
    socket,
    config: {
         prefix: '/cloudcmd' 
     }
}));

And use only prefix /console in server/cloudcmd.js:

config('console') && konsole({
    prefix: '/console',
    online,
});

The console's client side doesn't know where to lazy load it's data from.

it has a wrong prefix to load data from, only /console, so when it tries to load jquery (only console-io uses jquery now, but some time ago all parts that worked in modal window used jquery). And path to jquery becames:
/console/dist/modules/jquery/jquery.min.js and this is wrong because our root point is /cloudcmd.

What you want is a good idea, but we should think how to implement such behavior correctly :).

Could you please tell me how do you use Cloud Commander and how such code broke something for you?

app.use(cloudcmd({
    socket,
    config: {
        prefix: '/cloudcmd'
     }
}));

Maybe we can find another way for your case that will be much simpler :). The thing is all this stuff with prefixes made to avoid conflicts between Cloud Commander's files and middleware with any of your code, all this conditions:

if (url.indexOf(prefix))
    return next();

made for this purpose :).

[panlina]

Regarding express mount point, there is app.baseUrl.
You are right that client side still need to know the prefix, that's why I say server side:

You can eliminate most of the prefix tricks in server side source code

The client code doesn't need change the way they work. They keep prepending the prefix when they request resources, it's just no longer specified by the user of the middleware.
I believe the current way this middleware works has the same effect of mounting in most cases, but there's still some use cases I can think of where it does not satisfy:

• If I have a express app which loads modules as sub apps dynamically, it will have a name for each sub app, mount them at /${name}, and expect them to work. It doesn't know the prefix way.
• One can mount an app at several paths. It's as simple as app.use(['/cloudcmd', '/admin'], cloudcmd({..})). The prefix way doesn't benefit from it.

I'm not familiar with the client code right now. It's possible that I missed something. 😄

[coderaiser]

Added support of express mount point in v11.6.0 🎉.

Anyways you still need to set prefix for web sockets, we can change the way of doing things like this:

app.use(cloudcmd({
    config,  // config data (optional)
    plugins, // optional
    modules, // optional
}));

cloudcmd.listen(prefix, socket);
server.listen(port);

in next major release.

But it is two api calls, instead of one :).
Or generate some prefix. What do you think? Is it works for you?

[panlina]

This issue is fixed in v11.6.0, 😄, but seems prefix is still needed for middleware? Is it what you mean by saying:

Anyways you still need to set prefix for web sockets

?

app.use('/cloudcmd', cloudcmd({
	config: { prefix: '/cloudcmd' }	//it's still needed
}));

[coderaiser]

Yes, there is a need to set a prefix for web sockets.
express middleware it is not a place to init sockets connection :).

So as I said prefix should be generated or added with:

cloudcmd.listen(prefix, socket);

In next major release.
The thing is if it will be generated, client side should know somehow what the sockets prefix is.
So it is a good thing to think about :).

Anyways I do not think that it is a big problem to duplicate prefix with mount point, it is not very beautiful, but it works as expected.
Problem can occur when mount point with prefix are different and it is not very obvious. But again:

(req, res, next) => {
   // set prefix from req.baseUrl
}

not a very good place for validation as well, because it is a network request.

Do you have any ideas how to make things more simple and obvious?

[panlina]

Yes, I tend to agree with you. Call of socket listen should happen at the moment the server start to listen, but there seems no way for a middleware to be notified of this kind of event. I can't think of a better way either, so I can accept the duplicate.
For the same reason, a separate listen call seems good to me. It's too early to call listen in the middleware construction.
Good work on this useful app! 🎉

[coderaiser]

There is one more thing we can do about this, we can add one more conception prefixSocket, which will be used for websocket's connection only in dword, edward, deepword, gritty, fileop, console-io and client/modules/config.js (if I don't forgot something). All places where socket connect used.

In this case there will be no need to use mount point, prefix and prefixSocket at all. All of them will be optional. It can look like:

app.use('/cloudcmd', cloudcmd({
    config: {
        prefixSocket: '/cloudcmd' // optional
        prefix: '/cloudcmd', // will not be used if there is a mount point
    }
}))

Now prefix used for two things: web sockets connection and mount point, it is breaks single responsibility principle :).

[panlina]

Do you mean removing prefix(because it is not needed at all thanks to mounting), and introducing prefixSocket?

[coderaiser]

That is right prefix not needed when you are using mounting, but when you use Cloud Commander as a globally installed or deployed application, it is not required but can be used :).

[panlina]

--prefix can be used as a command line arg, but I think internally it should translate it into mounting also, to make it more consist.

cloudcmd/bin/cloudcmd.js

Line 182 in 51ee377

prefix: args.prefix || '', /* --no-prefix */

[coderaiser]

You absolutely right, and it is already translated to mounting point :). And with help of v11.7.0 you can use Cloud Commander as middleware with setting no prefix at all 😀🎉, just:

app.use('/cloudcmd', cloudcmd());

[panlina]

You're so quick. 😄. Will have a try.