Consider serving and transforming all HTML files, rather than just limiting it to index files

[rmhaiderali]

This pull request addresses two main issues.

Firstly, it resolves the problem of not serving HTML files in dev mode when the request explicitly includes filenames like /index.html or /main.html.

Secondly, in production mode, when specifically requesting /index.html, the HTML transformer function was being skipped, potentially causing issues as described in issue #109.

Additionally, this pull request will enable users to use the .htm extension in addition to .html. After applying this pull request, transformer functions can now be used with any file ending with .html or .htm.

This is achieved by skipping files with .htm or .html extensions in the Static Middleware Function to ensure proper serving and transformation of these files.

[rmhaiderali]

During my testing with .htm files, I discovered that Vite lacks support for this file format. I encountered the following issues:

1. Building with .htm files is not possible.
2. Vite treats .htm files as assets on the development server, leading to the absence of hot reloading functionality for these files.
3. While Vue functions well with .htm files (only in the Vite dev server), React plugin for Vite doesn't support .htm files at all.

Hence, we should also avoid transforming .htm files. In that case, the objectives of this pull request would be:

1. Enable the serving of HTML files with any name on the development server. Currently, it only serves index files, for example, accessing /main.html would result in a 404 error.
2. Allow the transformation of any HTML file when the request explicitly includes filenames such as /index.html or /main.html.

[szymmis]

Hi @rmhaiderali, thanks for your work.

Sorry to say that but I don't really like the idea of serving HTML files using their name.
I don't see any use-case for requesting page like a file, like you should just use /admin instead of /admin.html. That's because subpaths are also supported that way.
That's a lot of work for a little gain.

What I do agree though is that it might be misleading to have something.html files served but not transformed. That's why I do believe that html files should not be served as static files in production mode, like they are not currently served in development.

Let me know what do you think.

[rmhaiderali]

Hello @szymmis, I completely agree with your perspective on how things should ideally work—for instance, using /admin instead of /admin.html for cleaner and more semantic URLs. However, I believe that the goal is not just about serving HTML files by their names but also about providing a predictable and straightforward experience for developers. If we can find a balance that aligns with established practices while minimizing confusion, it could contribute to a more user-friendly environment for those working with Vite-Express.

Moreover, regarding subpaths, developers can leverage frontend routing libraries to handle explicit file name requests. For example, if a developer is handling subpaths, they likely have a routing library in place to manage navigation within their application.

By implementing a redirection strategy within the routing library, developers can ensure that all unmatched requests, including /index.html and /paththatneverexisted, are directed to the desired endpoint, such as / or /404. This way, even we will be serving html file, the frontend routing library can handle these requests appropriately.

On the other hand, if the serving of HTML files is disabled, a request to /index.html would result in a 404. This situation might be confusing, especially considering that all other requests, except those ending with .html, for instance, a request like /something.somethingOtherThanHTML would still be get transformed and served, contributing to potential inconsistency.

By serving and transforming all HTML files, we provide developers with enhanced control to effectively manage specific cases and allowing developers to maintain control over the application's routing behavior.

Let me know if you have any further thoughts or considerations on this approach.

[rmhaiderali]

Here is the reference where we can see vite is doing it in similar way.

[szymmis]

I'm always a fan of aligning vite-express with how Vite works and I understand your approach. If there is some edge case it might fix, I think it is indeed better to do it that way instead of just not serving html files as static files at all.

Also big thank you for cleaning up some unnecessary stuff. You are doing a really great job here at vite-express, thanks a lot!

[szymmis]

I feel like we need to find more meaningful name instead of findFilePath for that function. I'm not sure how those HTML files should be called, templates maybe?
So it would be called findTemplateFilePath or even readTemplate and it would replace those couple of lines (obviously after adding this functionality to the function itself 😅)

Suggested change

	const indexPath = findFilePath(req.path, config.root);
	if (indexPath === undefined) return next();
	const template = fs.readFileSync(indexPath, "utf8");
	const template = readTemplate(req.path, config.root);
	if (!template) return next();

What do you think about that?

[rmhaiderali]

What if we include the precise file path of the html file as an input for the getTransformedHTML function? This could assist users in distinguishing between various files, particularly when working with multipage apps. Additionally, I'm uncertain about the optimal parameter ordering for the getTransformedHTML function.

Suggested change

	const indexPath = findFilePath(req.path, config.root);
	if (indexPath === undefined) return next();
	const template = fs.readFileSync(indexPath, "utf8");
	let html = await server.transformIndexHtml(req.originalUrl, template);
	try {
	html = await getTransformedHTML(html, req);
	const templateFilePath = findTemplateFilePath(req.path, config.root);
	if (templateFilePath === undefined) return next();
	const template = fs.readFileSync(templateFilePath, "utf8");
	let html = await server.transformIndexHtml(req.originalUrl, template);
	try {
	html = await getTransformedHTML(
	html,
	req,
	templateFilePath.slice(config.root.length),
	);

[rmhaiderali]

What if we include the precise file path of the html file as an input for the getTransformedHTML function? This could assist users in distinguishing between various files, particularly when working with multipage apps. Additionally, I'm uncertain about the optimal parameter ordering for the getTransformedHTML function.

@szymmis what are your thoughts on this?

[szymmis]

Sorry for me being so slow to respond @rmhaiderali.

Answering your question: How would you want to change getTransformedHTML function to include additional parameter with the path to html? Because its responsibility is just transforming raw HTML string into another HTML string so it shouldn't know anything about file paths.

I guess we could merge findFilePath and getTransformedHTML functionalities into one function. But I'm not sure if there would be much benefit from that as there are small incompatibilities between two middlewares that are using those functions, so it might be tricky to do it in a clear way.

You don't need to do that much of a refactor, you can just change the function name or even leave it as is, because I'm planning to revisit the code in the near future with a little refactoring.

[rmhaiderali]

async function getTransformedHTML(
  html: string,
  req: express.Request,
  htmlFilePath: string,
) {
  return Config.transformer ? Config.transformer(html, req, htmlFilePath) : html;
}

The primary objective was to supply a precise HTML file path to a transformer function, enabling users to differentiate between various HTML files when dealing with multi-page applications. Subsequently, users can utilize this file path to apply distinct transformations to each HTML file or choose to apply transformations to specific files while excluding others.

For example if user have this structure:

├── index.html
└── nested1
    ├── index.html
    └── nested2
        ├─ index.html
        └── nested3
            ├─ index.html
            └── nested4
                └─ index.html

And they want to transform only /, /nested1/nested2/ and /nested1/nested2/nested3/nested4/ currently they have to do it like this:

ViteExpress.config({
  transformer: (html, req) => {
    if(req.path.startsWith("/nested1/nested2/nested3/nested4/")){
    // transform
    }
    else if(req.path.startsWith("/nested1/nested2/nested3/")){
    // unnessary check but required
    }
    else if(req.path.startsWith("/nested1/nested2/")){
    // transform
    }
    else if(req.path.startsWith("/nested1/")){
    // unnessary check but required
    }
    else if(req.path.startsWith("/")){
    // transform
    }
  }
});

But with this change they can do it like this:

ViteExpress.config({
  transformer: (html, req, htmlFilePath) => {
    if(htmlFilePath === "/nested1/nested2/nested3/nested4/index.html"){
    // transform
    }
    else if(htmlFilePath === "/nested1/nested2/index.html"){
    // transform
    }
    else if(htmlFilePath === "/index.html"){
    // transform
    }
  }
});

[rmhaiderali]

@szymmis, I've made changes to the variable/function names. I believe we can postpone adding filePath to the transformer function for now, and proceed with merging this pull request.

Thank you very much for your time and cooperation ❤️

[szymmis]

Okay I see now, but I think if you use regex instead of startsWith then those unnecessary checks would not be needed, isn't that correct?

I don't want to bloat the transformers in any way if there is an information already available in the request object. Also, your change would somehow tie request handling to how vite-express internally works with the HTML files etc. which is also something I don't like.

Nevertheless, after your changes to the naming everything looks great, so we can merge it! 🎉
Thank you for your another contribution! Looking forward to cooperate more in the future ❤️