Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to nuxt server and apply base path #1746

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

1nVitr0
Copy link

@1nVitr0 1nVitr0 commented May 1, 2023

Adresses #385, expanding on #1724

Updating nuxt to a server target went fairly well, although polishing (as always) revealed some quirks of nuxt:

  • The dynamic base path only works when specifying router base ./ at build time
  • The dynamic router base path is incompatible with @nuxt/pwa, so some open heart surgery was needed, thugh significantly more robust than the original pull request Fix: apply base path to client and websocket #1724
  • Nuxt inserts all nodes specified in nuxt.config.js => header BEFORE the <base> tag containing the router base path. This means, both Sortable lib and the favicon are not dynamic. I have created plugins for both of them (sortable, favicon), which I'm not very happy about. Especially loading the favicon after the scripts is not ideal.
  • The client must be run using nuxt start, I have not been able to get the supposed --standalone mode to work, so the client dependencies are necessary

In it's current state, it has been working throughout my local tests. I did run the same tests in the docker image, where for some reason the icon.svg as well as the Sortable.js lib return 404 errors. Not sure what the issue is here, but I've been staring at this for too long, probably just missing something simple.

In case you're wondering about the order changes in the Dockerfilec, this was mainly done for cache efficiency. I had the client dependencies install again every time I fiddled with the nuxt.config.js, so I split copying the package.json and the rest of the code. Similar deal with the other copy commands, I tried ordering from least to most often changed.

client/assets/app.css Outdated Show resolved Hide resolved
@advplyr
Copy link
Owner

advplyr commented May 1, 2023

How do you think we would run this in development with HMR on the client?

To get this working locally I went to the client and did npm run build.
Then started the server with client port and server port being the same.

When working on the client I rely on nuxt built-in HMR but I'm not sure how to get this working if we aren't able to run the client separately. I guess we could create a development path when running the server that doesn't use ClientRouter.

@advplyr
Copy link
Owner

advplyr commented May 1, 2023

I looked at sortablejs and it isn't being used. I'm not sure how that happened, I think originally I wasn't using vuedraggable or something like that.

@1nVitr0
Copy link
Author

1nVitr0 commented May 1, 2023

HMR should kick in when manually running "nuxt start" in the client folder. Unless I messed up the proxies. During tests I did manage to run them separately, though I might have changed things since then

Ah, you mean the proxies that were redirecting go absolutely paths? I was going to ask about those. What exactly needs to be provided where?

And yes, maybe just disabling the clientrouter start when running in debugging mode?

@advplyr
Copy link
Owner

advplyr commented May 1, 2023

What are you using for ClientPort and Port when running the server?

Are you doing npm run build then npm run start in the client which starts on port 3000.

Then npm run dev in the server with what for ClientPort and Port?

@1nVitr0
Copy link
Author

1nVitr0 commented May 1, 2023

Sorry, I think I totally missed your point there (I should really go to bed). You mean the client port when running nuxt in Dev Mode?

@advplyr
Copy link
Owner

advplyr commented May 1, 2023

Can you break down how you are running the client and server separately?

@1nVitr0
Copy link
Author

1nVitr0 commented May 1, 2023

Oh and I did run some benchmarks. It did look like it was running marginally slower than before. Which would make sense when not statically serving, and with the nuxt overhead. Will post tomorrow if I remember.

@1nVitr0
Copy link
Author

1nVitr0 commented May 1, 2023

Can you break down how you are running the client and server separately?

Can't copy paste, but of I'm not mistaken I used the launch script for the server (I commented out the clientRouter.start()). Should be the same as npm start

In the client dir I built nuxt using npm run build, and then ran it using ROUTER_BASE_PATH=/books CLIENT_PORT=3000 npx nuxt start. I'm pretty sure that was it. Unless I broke it when cleaning up for the pull request.

I do remember that nuxt kept shuffling the script order around. And changing the nuxt.config.js in different orders produced different results. That was great when debugging.

@1nVitr0
Copy link
Author

1nVitr0 commented May 1, 2023

Just jumped on the laptop for a second. I see the issue you're having. Because the port is different now, calls, e.g to /status do not get passed to the server when running separately, right? I think I will have to look into that tomorrow. Still need to copy over the websocket channel from the other pull request as well.

Yeh and the manifest is misbehaving again. Guess I'll still have to do some housekeeping tomorrow...

Sorry for the comment spam, just running npx nuxt dev seems to work for me.

@advplyr
Copy link
Owner

advplyr commented May 1, 2023

The problem for me is that running npm run start uses the NODE_ENV production instead of development. I made some updates that would still need to be cleaned up but here is how I ran dev.

Dev

Using 2 terminals to run client and server separately.

Client:
npm run dev in /client dir. No extra ENV variables passed in and this starts on port 3000 and expects the server to be on 3333.

Server:
npm run dev with my dev.js config setting Port and ClientPort to 3333. I don't think we need a separate ClientPort since we wouldn't be using the ClientRouter in development so I'm not sure why they would be different.

With the updates I made I'm able to use the same development environment as before.

Prod (locally)

Running in production locally to test with a different base path. For this I'm not running the client/server separately.

Client:
npm run build that is it because the client will be run from the ClientRouter

Server:
I updated index.js to allow for overriding the NODE_ENV so it can be set to production in my dev.js.
Then my dev.js looks like

module.exports.config = {
  Env: 'production',
  Port: 3333,
  RouterBasePath: '/abs',
  ClientPort: 3333
}

npm run dev
Now I can go to localhost:3333/abs

@1nVitr0
Copy link
Author

1nVitr0 commented May 2, 2023

Got you. I think. Though I couldn't get the server to work in production mode with the client sport the same as server port. Nuxt can't start, as 3333 is already in use. When keeping the client port, the client successfully connects to the server at :3000.

I haven't really tried running nuxt from loadNuxt and using render(route) instead of spawning a new process. Initially that caused so many errors that I went straight to running nuxt start. But that way we may only need the one port and can skip the proxy.

The manifest issue seems to be some kind of caching error, nuxt sometimes tries to load the wrong hash. I'll try to sneak in some updates throughput the day and report back.

@1nVitr0
Copy link
Author

1nVitr0 commented May 2, 2023

@nuxt/kit wasn't working, it didn't expose the for: 'start' option when using loadNuxt. I managed to switch to loadNuxt, imported from the client's nuxt dependency. That completely eliminates the need for a client port. And looks much cleaner in the process.

Running both the server, as well as nuxt in dev mode worked for me without any issues (apparently the manifest doesn't load in dev mode, yay). Thanks for your quick Feedback!

@1nVitr0
Copy link
Author

1nVitr0 commented May 2, 2023

As for performance, with using nuxt programatically it might even be a bit faster, though I'm not sure how lighthouse defines "Backend Latencies". JS Payloads are significantly reduced and server response times practically unaffected:

Static Desktop Static Mobile Server Desktop Server Mobile Δ
TTFB 1.7 ms 1.6 ms 3 ms 4 ms 🔺113 %
Redirects 163 ms 603 ms 0 ms 0 ms 🔹100 %
Largest Payload 2.99 MB 2.85 MB 866 KB 967 KB 🔹69 %
Unused JS 720 ms 4.800 s 70 ms 900 ms 🔹86 %
Backend Latencies 21 ms 22ms 407 ms 434 ms 🔺1856 %
FCP 2.4 s 14.0 s 1.1 s 5.4 s 🔹58 %

@1nVitr0
Copy link
Author

1nVitr0 commented May 2, 2023

@advplyr Of course I did not consider the debian packages after you explicitly told me about them. Thats also why the tests are failing. The --standalone option did absolutely nothing for me, so no automatic bundling of dependencies.

Including the node_modules from the client into pkg is. Eh. Especially as those might not be platform independent. That's probably the elephant that we missed when thinking about it. Currently fiddling around with it, maybe there is a simple solution.


Update:

Including the client's node_modules does allow the packaged executable to run (though it can obviously not alter the manifest.json in the virtual filesystem, that would've been the same in the previous pull request as well). Unfortunately this bloats the executable's size from 60MB to 270MB: 6d7c33e

},

// Global CSS: https://go.nuxtjs.dev/config-css
css: [
'@/assets/app.css'
],

favicon: '/favicon.ico',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything about this property in the nuxt docs. What does this do?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This currently gets read in the plugins/favicon.js plugin (the one I was not so happy about):

export default function ({ $config }) {
  const faviconPath = $config.favicon || '/favicon.ico'

  const link = document.createElement('link')
  link.rel = 'icon'
  link.type = 'image/x-icon'
  link.href = `${$config.routerBasePath || ''}${faviconPath}`

  document.head.appendChild(link)
}

@advplyr
Copy link
Owner

advplyr commented May 2, 2023

I built the executable on Windows and it is working well except for the manifest. I haven't looked into the manifest yet.

The windows binary is 114MB now and was 41MB before.

I also wasn't able to get the standalone to work. From my understanding this would allow us to bundle the client without all of the node modules used for building. The full node_modules directory is much bigger then we need.

@1nVitr0
Copy link
Author

1nVitr0 commented May 3, 2023

I haven't given up yet 😉 I may have a solution through @nuxt/bridge which backports some features from nuxt3. Including nitro deployment, which nicely bundles the dependencies to just under 2MB. But I'll have to do some very extensive testing to ensure nothing breaks.

@advplyr
Copy link
Owner

advplyr commented May 3, 2023

Awesome I didn't notice that Nuxt3 had the standalone server. https://nuxt.com/docs/guide/concepts/server-engine#standalone-server

nuxt/bridge looks like a perfect in-between step for us. Thanks for looking into all of this!

@1nVitr0
Copy link
Author

1nVitr0 commented May 4, 2023

I used nuxt3 back when it was in pre-before-almost-alpha. I found that feature pretty useful, but most modules and plugins were in a pretty broken state back then. But since they finally officially released v3, it does seem a lot better. I will commit an in-between version here just document my updates before I forget what I'm doing. I did have to update / alter / remove some packages due to incompatibility or they were no longer necessary:

  • UPDATE: epubjs to 0.5.0-alpha.0
  • UPDATE: @nuxtjs/tailwindcss to 6.6.7
  • REPLACED: @nuxtjs/pwa by pwa-nuxt-bridge
    • Was no longer maintained and incompatible with nitro, I have looked through the commits in the repo and nothing looked suspicous. Though we should check thoroughly before actually using it in production
  • REMOVED: @nuxtjs/proxy as the same functionality is provided by nitro now
  • ADDED: minimatch
    • was imported by @nuxtjs/tailwindcss, but not declared as dependency. Pinned to <v9.0.0, as v9.0.0 has no default export
  • ADDED: custom nitro preset
    • default node preset for some reason did not generate the assets and served the index.html on every route. node-server did, so I used that as a base.

It looks like it's mostly working, haven't been able to thoroughly check yet. Funnily enough the one thing I definitely didn't test, is if the base path updates lol. Docker container works as well.

@advplyr
Copy link
Owner

advplyr commented May 4, 2023

What version of node and npm are you using?

@1nVitr0
Copy link
Author

1nVitr0 commented May 5, 2023

16.20 Unless I forgot to change back from 18.x
I'll check in a second and if I do use 18 I'll try to downgrade again. That's what the Dev contaiber uses as well, right?

$ node -v
v16.20.0,

$ npm -v
8.19.4

@Hallo951
Copy link
Contributor

Hallo951 commented May 5, 2023

Just asking, what are the advantages of the change compared to the current solution?

I am not at all familiar with the individual server variants, but I am curious.

@1nVitr0
Copy link
Author

1nVitr0 commented May 5, 2023

@Hallo951 The issue we are fundamentally trying to tackle is #385 (sorry, didn't include that in the original description). I proposed #1724, built was not very robust and would not have worked when packaging into an executable. To make the solution more robust, I tried several routes and settled on running the client directly through node. We are currently in the troubleshooting phase...

Coincidentally, this now also is the first step on the way to eventually migrate to nuxt3, although this is not the intention of the pull request.

In case you were wondering why supporting a dynamic base path is important, it is a key ingredient when running multiple services behind a reverse proxy. E.g you could have running a webserver under https://your.domain/, abs on https://your.domain/audiobooks and plex under http://your.domain/mediaserver. Currently, this is not supported by abs, as when running in a base path other than /, all the assets, scripts and styles will still be loaded from / instead of the base path.

@Hallo951
Copy link
Contributor

Hallo951 commented May 5, 2023

Aha, I thought it had something to do with performance or stability. Thanks for the clarification, but after your explanation it's still not quite clear to me what it's for. Have I understood correctly that it is essentially about the integration of ABS into websites? What does this nuxt3 do differently from the current implementation?

@1nVitr0
Copy link
Author

1nVitr0 commented May 5, 2023

nuxt3 is just the newest version of Nuxt, which is the framework ABS uses for it's client. The currently used version 2 is still supported, and even receives semi-regular updates (the last one was about 2 months ago). So it's not an immediate priority to upgrade to the next version. However, nuxt2 is still based on Vue version 2, which has not received any updates in more than 6 months and also includes some other packages, that are no longer maintained or stuck on a version that still supports vue2. Upgrading a few packages and migrating to nuxt3 will definitely improve performance of the client. But I think it's not that much of a big deal, as ABS does not care about SEO or cutting-edge load times.

All that is a side-effect though, the original issue is just that you cannot change the base path for ABS at the moment. Currently ABS uses nuxt generate and builds a static version of the client. That means, it creates a collection of html, js and css files that can simply be served by a web server. These static files do not support changing the base path, so a more dynamic runtime solution is needed. nuxt offers another standard way by nuxt build which builds the client to a collection of executable jsfiles instead. These support more dynamic environments, such as changing the base path.

However, nuxt2 builds these files without bundling the dependencies. This means, the entire node_modules folder must be shipped in order to run the client. This would bloat both Linux or Windows installable executables, as well as the Docker images. To solve this, we can go part of the way to using version 3 of nuxt by including @nuxt/bridge. This is the official intermediate step between version 2 and 3 and supports some of the improvements nuxt3 offers. Including bundling the dependencies of the nuxt build step, so the node_modules folder is no longer necessary. And that's the step we are currently at.

I do think using @nuxt/bridge might offer some solutions for future pull request while paving the way for an eventual upgrade to nuxt version 3. So it's not just throwing everything out in order to support a single feature. But it's definitely a more fundamental change than I would have thought when starting to work on this issue.

@advplyr
Copy link
Owner

advplyr commented May 5, 2023

Great summary of where we are at.
Yesterday I was trying to run the client by itself using npm run dev and it was getting stuck in an infinite loop. It looks specific to Windows.
Using node v16.14.0 and npm v8.3.1

I didn't have much time yesterday to try anything

Here was the issue I saw making me think it is Windows specific nuxt/bridge#84

@1nVitr0
Copy link
Author

1nVitr0 commented May 5, 2023

@advplyr I have to restart my system in order to test Windows, so great that you're doing it. Is nuxt also eating up your commandline or is this a me-issue? I mean, it's great, all the warnings and errors are gone. But only because a very hungry nuxt progress bar solwly makes it's way up lol...

I also get a TypeError: Invalid host defined options when trying to build and run pkg

@advplyr
Copy link
Owner

advplyr commented May 5, 2023

Yep nuxt is eating up my commandline. It makes it difficult to catch errors. Currently I had those errors mentioned in that issue I linked

@1nVitr0
Copy link
Author

1nVitr0 commented May 5, 2023

I piped the output to a file, that way only errors showed up and stayed there.

npm run build > output.txt

Not sure if that works on windows

@1nVitr0
Copy link
Author

1nVitr0 commented May 5, 2023

Ah, pkg can't deal with ES modules. Gotta love the transition from commonjs to modules, always a great time. I'm sure I can somehow coax rollup to generate commonjs. The options are all there (I think), just have to sift through the Plugins nitro injects...

@advplyr
Copy link
Owner

advplyr commented May 5, 2023

Ugh yeah. We may also find an alternative to pkg.
I'll be traveling for the next week and may not be as responsive as normal.

@1nVitr0
Copy link
Author

1nVitr0 commented May 6, 2023

No worries, don't think we should rush this. If you're not attached to pkg I'll be on the lookout for an alternative. Though even the upcoming node 20's Single executable applications currently only support commonjs...

@1nVitr0
Copy link
Author

1nVitr0 commented May 9, 2023

It will take some time to explore the options we have when packaging the application. There aren't many alternatives that are as plug and play as pkg. There is nexe, but that's still stuck at node 14. I do think the best option is somehow transpiling the dependencies of the client to commonjs. I have gotten the server code itself as commonjs already, and with nitro, there are actually only 18 dependencies, 13 of which are esm modules.

Hopefully I'll have something by the end of the week. Electron works with esm, maybe I can rip out something from there...

@1nVitr0 1nVitr0 force-pushed the feature/nuxt-target-server branch from cc6e6a8 to 0c0b981 Compare May 10, 2023 20:32
@1nVitr0
Copy link
Author

1nVitr0 commented May 10, 2023

I thought way too complicated. esbuild already has it all. Not sure what kind of black magic voodoo nitro does behind the curtains, but it doesn't expose all esbuild options. And it looks like that's not even the last step in their build chain.

Anyway, when appending another esbuild step after nitro, it all packages nicely into a single commonjs file that can be required in the ClientRouter. I can confirm that this even works without any alterations to the pkg packaging (apart form less required files). I do think this is fairly clean and should be pretty robust, definitely better than the convoluted mess I had in mind. Even the tests should now run (I think).

I will now work (again) on the actual issue I wanted to solve, I'm pretty sure I broke the base path again. Also managed to produce some manifest and icon errors again again. But now that I have a functioning base to work on it shouldn't be too hard.

@1nVitr0 1nVitr0 force-pushed the feature/nuxt-target-server branch from 0c0b981 to 6094b99 Compare May 10, 2023 20:51
@1nVitr0 1nVitr0 force-pushed the feature/nuxt-target-server branch from 6094b99 to 03ed99d Compare May 10, 2023 20:55
@1nVitr0
Copy link
Author

1nVitr0 commented May 15, 2023

Short update: Nuxt in server mode still prerenders all pages as html. That means the scripts and assets all initially load from the wrong basePath. More fiddling around. Looks like this will take a while. But at least it should prepare wuite well for nuxt3.

@advplyr
Copy link
Owner

advplyr commented May 15, 2023

Just got back to my home office so I can help with testing again.
I'm not sure what you mean about Nuxt in server mode.
It was working when we were bundling all of the node_modules before switching to nitro. Is this a change introduced with nitro or using the standalone server build option is doing this?

@1nVitr0
Copy link
Author

1nVitr0 commented May 16, 2023

I'm not sure anymore what nitro changed. I will roll back to the previous attempt and test tonight (that's in about 9 hours). Now that I have worked with the nuxt esbuild combination more intimately, it might be possible to bundle the non-bridge version into a single file. Though it did work very differently without bridge (requiring loadNuxt instead of directly requiring).

I think I was going through the html files and replacing things before, but maybe I am confusing that with the initial attempt. When packaging for Linux / WIndows we can no longer rewrite files, as any method of packaging will introduce a read-only virtual filesystem. Unless we want to write a massive package-building gulp script, but I'm not ready for that...

@1nVitr0
Copy link
Author

1nVitr0 commented May 18, 2023

You were right, I solved it by using relative base paths. I can do the same with nuxt. Manifest ist still an issue, but that can be a solve.

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.

None yet

3 participants