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

Export getViteConfig #124

Merged
merged 5 commits into from
Mar 23, 2024
Merged

Export getViteConfig #124

merged 5 commits into from
Mar 23, 2024

Conversation

srguiwiz2
Copy link
Contributor

For issue #123 .

Copy link
Owner

@szymmis szymmis left a comment

Choose a reason for hiding this comment

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

Hi @srguiwiz2, thanks a lot for your suggestion and contribution. I think we can do that. I was hesitant at first but I guess it makes sens to have it available outside.

I have only one request, can you expand the example in the documentation to show the whole snippet with the usage you are describing? I think it will tell a lot more than just a sample of instantiating this variable.

@srguiwiz2
Copy link
Contributor Author

srguiwiz2 commented Mar 13, 2024

I'd make another commit and in the example code for documentation change the constant name to viteExpressViteConfig instead of viteExpressConfig to be more correctly describing what it is.

Some people don't like too many commits, or long names, etc., so I'm asking before I actually commit. I prefer unambiguous names, even if they end up being longer.

@srguiwiz2
Copy link
Contributor Author

The ambiguity I am trying to avoid with that long name may become more clear with the actual complete server.js it will be used in:

import express from "express";
import ViteExpress from "vite-express";
import { handler as apiHandler } from "./api.js";

const listeningPort = 3000;
const listeningHostname = "localhost";

const viteExpressConfig = {};
// see https://github.com/szymmis/vite-express?tab=readme-ov-file#-shipping-to-production
// see https://nodejs.org/en/learn/getting-started/nodejs-the-difference-between-development-and-production
switch (process.env.NODE_ENV) {
  case "development":
  case "production":
    viteExpressConfig.mode = process.env.NODE_ENV;
    break;
  default:
    viteExpressConfig.mode = "development";
}
ViteExpress.config(viteExpressConfig);

const expressApp = express();
expressApp.use(apiHandler);
const expressServer = expressApp.listen(listeningPort, listeningHostname,
  async () => {
    const viteExpressViteConfig = await ViteExpress.getViteConfig();
    const appUrl = `http://${listeningHostname}:${listeningPort}${viteExpressViteConfig.base}`;
    console.log(`Serving app from root ${viteExpressViteConfig.root} in ${viteExpressConfig.mode} mode`);
    console.log(`App server is listening at ${appUrl}`);
  });
ViteExpress.bind(expressApp, expressServer);

But unless you say otherwise I would think that is too long as an example for the main documentation.

@srguiwiz2
Copy link
Contributor Author

srguiwiz2 commented Mar 13, 2024

And for completeness, that server.js goes with package.json containing:

  "scripts": {
    "dev": "vite",
    "dev-server": "node ./server.js",
    "build": "vite build",
    "preview": "vite preview",
    "preview-server": "vite build && NODE_ENV=production node ./server.js",
  },

@srguiwiz2
Copy link
Contributor Author

I have built a bit more scripting around using this feature for automatic handing of serving. Too much to list here, but...

Noteworthy is one very short script to run as part of the build process that only uses const viteExpressViteConfig = await ViteExpress.getViteConfig(); and then writes base and outDir into a file, so that if later in viteless mode those values can be used for an inlineViteConfig and thereby avoid the need to actively maintain the same base in two places.

@srguiwiz2
Copy link
Contributor Author

Hi @szymmis, if you approve this PR for the one extra export then I can recommend this package in other discussions.

Or, do you want me to make another change before approving?

It is a good package anyway. For us here, this extra export makes it nicer to automate.

Thank you.

README.md Outdated
@@ -393,6 +394,32 @@ Used when you want to build the app to production programically. It is adviced t
ViteExpress.build();
```

### `async getViteConfig() => Promise<ViteConfig>`

Useful in case you want to write a specific `console.log` message in the listen callback, for example to construct a URL using the base too.
Copy link
Owner

Choose a reason for hiding this comment

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

I would change that to:

Useful in case you want to know the Vite configuration parameters currently in use, for example when you want to write a specific console.log message with that information in the listen callback.

Something like that, otherwise it looks good!

@szymmis
Copy link
Owner

szymmis commented Mar 22, 2024

Hi @srguiwiz2, sorry for taking so long to answer but I'm a little bit swamped lately.
I've written one comment in the review, otherwise that looks great.
Thanks for your contribution and I'm always happy to hear that you guys are having a good time using this tool.

@rmhaiderali
Copy link
Contributor

rmhaiderali commented Mar 22, 2024

Hey @srguiwiz,

Could we simplify the example in the README a bit? Making it easier to understand and aligning it with other examples would be great.

import express from "express";
import ViteExpress from "vite-express";

const app = express();

const port = 3000;
const host = "localhost";

const server = app.listen(port, host);

ViteExpress.bind(app, server, async () => {
  const { root, base } = await ViteExpress.getViteConfig();
  console.log(`Serving app from root ${root}`);
  console.log(`Server is listening at http://${host}:${port}${base}`);
});

@szymmis
Copy link
Owner

szymmis commented Mar 22, 2024

I really like that what you did @rmhaiderali. There is less code and definitely more clarity in that snippet now. What do you think @srguiwiz2?

@srguiwiz2
Copy link
Contributor Author

srguiwiz2 commented Mar 22, 2024

Nice. @szymmis I can insert as suggested by @rmhaiderali. I have verified it too by running it.

Shall we instead of ${port} make that ${server.address().port} to cover getting a random port number when passing 0? You decide, then I can commit.

@srguiwiz
Copy link
Contributor

Committed with plain ${port} for brevity.

Copy link
Owner

@szymmis szymmis left a comment

Choose a reason for hiding this comment

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

Everything is perfect now, thanks!

@szymmis szymmis merged commit e1a3719 into szymmis:master Mar 23, 2024
1 check passed
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.

4 participants