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

addPreprocessor doesn't work together with keyed addExtension #3433

Closed
mayank99 opened this issue Sep 8, 2024 · 15 comments
Closed

addPreprocessor doesn't work together with keyed addExtension #3433

mayank99 opened this issue Sep 8, 2024 · 15 comments
Labels

Comments

@mayank99
Copy link
Contributor

mayank99 commented Sep 8, 2024

Operating system

macOS 14

Eleventy

3.0.0-alpha.19

Describe the bug

I'm trying to use addPreprocessor to transform the input file contents before handing it off to a template renderer.

This works for simple cases, but I've hit a wall when trying to use it with keyed addExtension (for aliasing an existing template language). Specifically, I'm trying to use key: "11ty.js" so that I can access this.defaultRenderer inside the compile function. What happens is 11ty tries to process this template too early, before it has been transformed by the logic in addPreprocessor, which means it fails when it encounters unsupported syntax.

[11ty] Problem writing Eleventy templates:
[11ty] Unexpected token (3:4) (via SyntaxError)

Reproduction steps

See stackblitz.

Note: I'm only using "js" to illustrate the problem more clearly. Using "jsx" or "tsx" also fails, but with a different "Unknown file extension" error (possibly related to #3393 (comment)).

Note 2: A similar problem also happens in Deno (which natively supports JSX/TSX), except there is no error; the content in addPreprocessor is just empty. See minimal repro.

Expected behavior

addPreprocessor should execute before setting up a template alias (i.e. using addExtension with key). Both features should be usable together.

Reproduction URL

https://stackblitz.com/edit/github-qaud3e?file=eleventyJsxPlugin.js

Screenshots

No response

@zachleat
Copy link
Member

Hmm. Agree that this is a bug. Currently we handle template types that read file content separate from dynamic import of JavaScript templates (through the 11ty.js template type, aliased or not).

The most correct way to fix this is to restructure JavaScript templates to import during the file read step, which feels like a refactor that’s a little big for this late in the 3.0 release lifecycle.

This is the place where it forks:

I’ve pushed a branch that has the fixes in place but I’m nervous to make this change 😅 https://github.com/11ty/eleventy/compare/issue-3433?expand=1

@mayank99
Copy link
Contributor Author

mayank99 commented Sep 15, 2024

So I tried the changes from e221e19 locally by editing node_modules, but it still doesn't seem to fix the issue. I get the same SyntaxError in Node, and the content is still empty in Deno.

Diff
diff --git a/src/Engines/JavaScript.js b/src/Engines/JavaScript.js
index b20db4e0dd43f8305aba431989a4f67d404017cd..568bff12a4c8aac24020f89528d586a6b04165e1 100644
--- a/src/Engines/JavaScript.js
+++ b/src/Engines/JavaScript.js
@@ -123,6 +123,10 @@ class JavaScript extends TemplateEngine {
 		return false;
 	}
 
+	useJavaScriptImport() {
+		return true;
+	}
+
 	async getExtraDataFromFile(inputPath) {
 		let inst = await this.getInstanceFromInputPath(inputPath);
 		return getJavaScriptData(inst, inputPath);
diff --git a/src/Engines/TemplateEngine.js b/src/Engines/TemplateEngine.js
index 05050b68b043a9a7d8383fbb7d3944642f339091..75937abe974b1fca36e75ff3ef0415dfed02ffbd 100644
--- a/src/Engines/TemplateEngine.js
+++ b/src/Engines/TemplateEngine.js
@@ -116,6 +116,10 @@ class TemplateEngine {
 		return true;
 	}
 
+	useJavaScriptImport() {
+		return false;
+	}
+
 	getExtraDataFromFile() {
 		return {};
 	}
diff --git a/src/TemplateContent.js b/src/TemplateContent.js
index 143d958282d43348c31b5f0d970f4b03d36d567c..2e00ca60fc8d42797b9e316227506b4221ac8a86 100644
--- a/src/TemplateContent.js
+++ b/src/TemplateContent.js
@@ -184,6 +184,12 @@ class TemplateContent {
 		let content = await this.inputContent;
 
 		if (content || content === "") {
+			if (this.engine.useJavaScriptImport()) {
+				return {
+					data: {},
+					content,
+				};
+			}
 			let options = this.config.frontMatterParsingOptions || {};
 			let fm;
 			try {
@@ -266,6 +272,14 @@ class TemplateContent {
 
 	async getInputContent() {
 		let tr = await this.getTemplateRender();
+
+		if (
+			tr.engine.useJavaScriptImport() &&
+			typeof tr.engine.getInstanceFromInputPath === "function"
+		) {
+			return tr.engine.getInstanceFromInputPath(this.inputPath);
+		}
+
 		if (!tr.engine.needsToReadFileContents()) {
 			return "";
 		}

@zachleat
Copy link
Member

Hmmmmmm—can you supply your test case?

Here’s what I tested with (with the changes) and it had content for both the .11ty.js and .test templates:

https://gist.github.com/zachleat/c54611d86ab2e5c521802b211904eb65

@mayank99
Copy link
Contributor Author

mayank99 commented Sep 17, 2024

Ah, I mentioned this in the issue description (see "Note 2"). Here's a proper link to the branch which fails in Deno: https://github.com/mayank99/11ty-jsx-deno/tree/preprocessor

One difference I noticed from your gist is that I also pass a compile function to addExtension. If I remove the compile function, then content in addPreprocessor is populated but it is an object containing a render function, rather than being the actual stringified contents of the file.

{ render: [Function: Page] }

I also have a repro case for Node (without Deno) in the issue description. That one continues to fail with SyntaxError even before the addPreprocessor code is executed.

@zachleat
Copy link
Member

Ah—my apologies for bad reading comprehension! 😭 I’ll have a second look!

@zachleat
Copy link
Member

I pushed some additional changes to the issue-3433 which I believe will fix the issue—can you retest? https://github.com/11ty/eleventy/compare/issue-3433?expand=1

@mayank99
Copy link
Contributor Author

Thanks for looking into this again, Zach. I have some good news and more bad news.

I tested out the new changes against both repros (Node stackblitz and Deno branch) and here's what I found:

  1. There is no change in Node behavior - it is still throwing a SyntaxError.
  2. In Deno, the content argument of addPreprocessor is no longer empty (🎉), but it is still not a string. Instead, it is returning an object containing a render function (😭), similar to my previous comment. Unless I'm misunderstanding addPreprocessor, I don't think this is expected?

@zachleat
Copy link
Member

zachleat commented Sep 18, 2024

Hmm, we’re iterating towards something :D

Let’s start with the Node behavior first:

I think there is some confusion about the 11ty.js format—it doesn’t accept strings nor execute string input. Adding a virtual template as an 11ty.js template is just passing in JavaScript. So I wouldn’t expect that the content passed to addPreprocessor to be a string (nor aliases via addExtension)—I would expect it to be JavaScript at that point.

When you remove key: '11ty.js' from your alias, it treats the template as plaintext and then addPreprocessor will pass in a string.

Worse, I wouldn’t expect returning a string from addPreprocessor to execute a string of JavaScript either. We have a similar limitation executing arbitrary JS in the Render plugin https://www.11ty.dev/docs/plugins/render/#rendertemplate

I think I would classify that as a bigger enhancement request (not a bug right now).

All that said, we do use the node-retrieve-globals package to implement the js front matter type to execute arbitrary JavaScript #2819 so there is a limited precedent for a path forward here.

Finally, I would call out the method used by tsx https://github.com/privatenumber/tsx (likely some magic using module.register https://nodejs.org/docs/latest/api/module.html#moduleregisterspecifier-parenturl-options but I haven’t dove all the way in to see) that might be a more ergonomic way to intercept and preprocess JavaScript. This is what we recommend on https://www.11ty.dev/docs/languages/typescript/

Open to suggestions here (of course).

@mayank99
Copy link
Contributor Author

mayank99 commented Sep 19, 2024

Yeah, it sounds like I might have misunderstood what addPreprocessor is supposed to do. I was under the impression that I'd be able to do arbitrary transformations to the source code before Eleventy tries to render (or do anything with) it. There was still a bug here though, at least in Deno, and it seems to have been fixed by some of the changes you shared above.

I've definitely looked into tsx (and esbuild-register), however I find these libraries to be too limiting. I want more control over the transformation, which is why I was looking into addPreprocessor. That's a good callout on module.register though, I'll look into that.

I do wonder if it would be easier to recreate the 11ty.js renderer (instead of reusing it via key: "11ty.js"). Then I could treat it as a fully custom template language which can be preprocessed as a string. Do you have an idea how much work it would be to re-implement 11ty.js-like compilation on custom templates? Is 11ty doing something more involved or is it just a dynamic import of the actual file on disk? I was thinking I could even use node-retrieve-globals as a starting point to extract the default export from the stringified code; but then it wouldn't handle things like imports.

Of course, the last resort is to run the pre-transform and 11ty as two separate steps, but I was hoping to avoid producing an intermediate set of files.

@zachleat
Copy link
Member

zachleat commented Sep 19, 2024

I think you can avoid a lot of abstraction complexity tied to node-retrieve-globals since you already have esbuild in play (specifically the extra restrictions Node.js has placed on vm and ESM together) by outputting CJS code from esbuild and using Node’s Module#_compile (dynamic CJS) API directly.

Something like this:

import path from "node:path";
import { pathToFileURL } from "node:url";
import { Module } from "node:module";
import { renderToStringAsync } from 'preact-render-to-string';
import { isValidElement } from 'preact';
import * as esbuild from 'esbuild';

let elev = new Eleventy("./test/stubs-jsx/", undefined, {
  config: eleventyConfig => {
    eleventyConfig.addTemplateFormats('11ty.jsx');

    eleventyConfig.addPreprocessor(
      'jsx-transform',
      ['11ty.jsx'],
      async (data, content) => {
        const { code } = await esbuild.transform(content, {
          loader: 'jsx',
          jsx: 'automatic',
          jsxImportSource: 'preact',
          format: 'cjs',
        });

        let m = new Module();
        let workingDir = pathToFileURL(path.resolve(".")) + "/";
        m.paths = Module._nodeModulePaths(workingDir);
        m._compile(code, workingDir);
        return m.exports;
      }
    );

    eleventyConfig.addExtension(['11ty.jsx'], {
      compile: function (content) {
        return async (data) => {
          let page = content.default();
          if (isValidElement(page)) {
            return `<!doctype html>${await renderToStringAsync(page)}`;
          }
          return content;
        };
      },
    });
  }
});

Then you could use import without worry, a la:

import { noop } from "@zachleat/noop";

export default function Page() {
	return (
		<div>Hello!</div>
	);
}

zachleat added a commit that referenced this issue Sep 19, 2024
@zachleat
Copy link
Member

Regardless, I’ll have a deeper think about the additional changes made in this PR and slate it for 3.0 for now: #3452 I do think we made forward progress there—thank you!

@zachleat zachleat added this to the Eleventy 3.0.0 milestone Sep 19, 2024
@mayank99
Copy link
Contributor Author

mayank99 commented Sep 19, 2024

Oh wow, TIL about Module#_compile. This looks like an undocumented/private API, which does make me nervous but otherwise it's pretty much exactly what I need. Between this and register, I think I have a clearer path forward. Thank you!

@zachleat
Copy link
Member

It is an off-the-books API but everyone uses it. I originally found it via Svelte’s repo I think? Pour one out for nodejs/node#40098

@zachleat
Copy link
Member

Just published a little research on the different methods that I know of for dynamic script execution in Node.js here: https://github.com/zachleat/node-eval-modules

@zachleat
Copy link
Member

Shipping with 3.0.0-alpha.21 and 3.0.0-beta.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants