eeue56 / elm-static-html-lib

BSD 3-Clause "New" or "Revised" License
57 stars 11 forks source link

allow grouping multiple view functions into one elm file #8

Closed folkertdev closed 6 years ago

folkertdev commented 6 years ago

my use case

I would like to use this library to generate a bunch of html templates (for hakyll). They all depend on the same style code, so in most cases, I want to regenerate all my templates when I change some elm code.

Currently, putting all these view functions (about 10) in a .js file and calling this lib for all of them is too slow (5 seconds), because 10 elm files are generated and compiled (it also seems that my code as it is imported, is compiled multiple times).

proposed fix

Allow for putting multiple view functions together, compiling them into one elm file. This PR introduces

export interface ViewFunctionConfig {
    viewFunction: string;
    output: string;
    model?: any;
    decoder?: string;
    indent?: number;
    newLines?: boolean;
}

export function grouped(
    rootDir: string, moduleName: string, configs: ViewFunctionConfig[],
    alreadyRun?: boolean, elmMakePath?: string, installMethod?: string): Promise<void[]> 

the moduleName argument is only used to generate a unique hash. Might be better to concatenate the view function names and take that as the hash)

This function does the writing to file for you, because otherwise the user needs to keep track of the order of the view functions and then reassociate the rendered html with the correct output file later.

Alternatively we could still take the filename as an input but give back a Promise<[[string, string]]> where the first element is the rendered html and the other is the file name.


Does this seem good architecturally? the PR actually contains code that implements this idea but it's not very pretty yet and I'd like to check whether this is the right idea first.

folkertdev commented 6 years ago

I've switched to returning a Promise<[string]>. The extra flexibility of having a list of strings to manipulate is probably worth it.

To make grouped work together with other elmStaticHtml calls, some further changes were needed.

It seems that elm's port namespace is global, so even though we can compile multiple main functions into one javascript file, we can't have multiple ports with the same name.

There are then two solutions: only generate the ports once (storing them in some Ports.elm file, or generating multiple .js files. The second option is simpler and thus the one I went with for now. I vaguely remember hearing something about this problem, so maybe this issue is solved in 0.19?

folkertdev commented 6 years ago

I've started work on the single-port approach we discussed on slack. It makes some of the problems with hashes you just highlighted go away and cleans up the code considerably.

I hope to have that working and incorporate the above comments by saturday. Thanks for the review

eeue56 commented 6 years ago

Thanks for the PR!

folkertdev commented 6 years ago

found some time today

The above commits

One further idea: compile into separate .js files. This diff gives a 30% speed increase (5.1 to 3.4 seconds) on my machine on examples/index.ts. Including all the PrivateMain files results in n! compilations, compiling just to the PrivateMain needed is constant.

diff --git a/index.ts b/index.ts
index 7a3e49f..0a2228f 100644
--- a/index.ts
+++ b/index.ts
@@ -39,7 +39,7 @@ function parseProjectName(repoName: string): string {
 function runElmApp(moduleHash: string, dirPath: string, filenamesAndModels: any[][]): Promise<Output[]> {

     return new Promise((resolve, reject) => {
-        const Elm = require(path.join(dirPath, "elm.js"));
+        const Elm = require(path.join(dirPath, `elm${moduleHash}.js`));^M
         const privateName = `PrivateMain${moduleHash}`;

         if (Object.keys(Elm).indexOf(privateName) === - 1) {
@@ -218,7 +218,7 @@ function runCompiler(moduleHash: string,
                      configs: ViewFunctionConfig[], elmMakePath?: string): Promise<Output[]> {
     const options: any = {
         cwd: rootDir,
-        output: "elm.js",
+        output: `elm${moduleHash}.js`,^M
         yes: true,
     };

@@ -228,7 +228,7 @@ function runCompiler(moduleHash: string,

     return new Promise((resolve, reject) => {
         fs.readdir(rootDir, (err, files) => {
-            const actualFiles = files.filter((name) => name.indexOf("PrivateMain") === 0);
+            const actualFiles = files.filter((name) => name.indexOf(`PrivateMain${moduleHash}`) === 0);^M

             const compileProcess = compile(actualFiles, options);
             compileProcess.on("exit",

This means more files are generated (and the cache removal needs to be a bit more complex) but for 30% speed gain it seems worthwhile. thoughts?

eeue56 commented 6 years ago

One further idea: compile into separate .js files. This diff gives a 30% speed increase (5.1 to 3.4 seconds) on my machine on examples/index.ts. Including all the PrivateMain files results in n! compilations, compiling just to the PrivateMain needed is constant.

Why is this faster? 🤔

folkertdev commented 6 years ago

The compilation is slow because elm files are needlessly compiled multiple times.

say we will generate 3 privatemain files

So 3 elm files results in 1 + 2 + 3 compilations (this is n^2 complexity). Ideally, you'd want to only generate JS once all the elm is generated, but that is impossible to enforce with the singular elmStaticHtml commands.

grouped works around this problem by only generating one PrivateMain for multiple views, but we can solve the problem in general by generating a elm.js file per elm file.

eeue56 commented 6 years ago

Gotcha. I think we should optimize for runtime, rather than compile time. Maybe that can be a different PR later on?

folkertdev commented 6 years ago

are multiple applications compiled into one file faster than separate applications? Otherwise there would only be more disk space used, the runtime is unchanged.

but yes this can be a separate thing.

eeue56 commented 6 years ago

are multiple applications compiled into one file faster than separate applications? Otherwise there would only be more disk space used, the runtime is unchanged.

Yep -- requiring a single file in the JS side is faster than requiring as needed

folkertdev commented 6 years ago
eeue56 commented 6 years ago

Published as 0.1.0! Thanks for your hard work 🎉