Fix: Removes a misleading log message telling that a custom renderer is not recognized while it clearly is. Adding a new example to demo custom renderers for prove. #12461

kyr0 · 2024-11-18T02:14:11Z

Changes

What does this change?

Removes the misleading log message telling that a custom renderer is not recognized while it clearly is and works.
Also adds a framework-custom example that demonstrates how to write custom renderers.
The example proves that custom renderers work well with the change.

Before:

After:

Fixes: #12463

The only change that is Astro core related is:

https://github.com/withastro/astro/pull/12461/files#diff-f28699aad5f8ec8f0ab34b5e512b815bf3088a32898cbeb1368b6be5d27a9813

It should be safe to merge, because no logic other than a log message is affected.

Testing

pnpm --filter @example/framework-custom run dev

Enjoy! :)

Docs

I don't think that this use-case is relevant to most developers. This is a change that affects the flexibility of Astro to welcome and accept custom framework / custom markup implementations. Whoever is interested in building such advanced integrations, will probably take a look at the code first and find the new example that comes with this PR which clearly demonstrates how to do so.

…atic list of renderers while Astro allows to dynamically add custom renderers

… renderers with the log message fixed

…nce (real-world use)

changeset-bot · 2024-11-18T02:21:53Z

🦋 Changeset detected

Latest commit: b9f2d45

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

kyr0 · 2024-11-18T03:05:43Z

The only reason why the tests are failing seems unrelated to my code:

ematipico · 2024-11-18T09:20:15Z

We usually use examples for beginners who want to start from somewhere. Having a custom renderer is not a beginner example, so I don't think we should have it. Instead, we should have tests that cover the new changes.

ematipico

I would remove the example and add a new test

kyr0 · 2024-11-18T19:12:50Z

@ematipico Right, I was foreseeing this argument somehow :)) I'll take the example code and put it in a new repo under my Github account. People who plan to implement their own custom renderers would be able to find our conversation, and start from there.

I'll also add a test to verify my changes, but due to the nature of the issue, I need to temporarily hook the console.warn function in order to prove that the log is not written anymore.

florian-lefebvre · 2024-11-18T19:22:58Z

If that can help, in this commit I removed a patch to intercept console logs. Maybe you can reuse it in some way

kyr0 · 2024-11-19T03:19:57Z

Moved the example code for a custom renderer / custom frontend framework integration over here: https://github.com/kyr0/astro-custom-renderer

codspeed-hq · 2024-11-19T04:42:24Z

CodSpeed Performance Report

Merging #12461 will not alter performance

_{Comparing kyr0:fix/custom-renderer-warning (b9f2d45) with main (a23985b)}

Summary

✅ 6 untouched benchmarks

kyr0 · 2024-11-19T05:18:34Z

To be honest: I don't believe so. Removing a console.warn certainly wouldn't drop the performance by 11%; especially not if the codepath affected by the report hasn't be changed at all.

It seems like CodSpeed has a higher variance than promised?

ematipico · 2024-11-20T08:00:52Z

Moved the example code for a custom renderer / custom frontend framework integration over here: kyr0/astro-custom-renderer

If you plan to advertise your custom renderer example, I would suggest to change the entry points to use a valid extension. Currently they don't have one, which is invalid in Node.js. Every import must have a file extension. In fact, even our container API suggests the use of the extension

bluwy · 2024-11-21T16:02:25Z

packages/astro/test/custom-renderer.test.js

+		it('does not log a warning message when using a custom renderer', async () => {
+			const res = await fixture.fetch('/');
+			assert.equal(res.status, 200);
+
+			assert.equal(
+				warnLogsWritten.indexOf(
+					'If you intended to use a different renderer, please provide a valid client:only directive.',
+				),
+				-1,
+			);
+		});
+	});


I think I'd prefer removing this test since it's only guarding if we ever revert this PR, which if we do we probably have a good reason for it. That way we also avoiding patching console.warn for this test.

The other test cases are fine to keep though, we're lacking tests for custom renderers.

@bluwy I agree, but then I cannot test if the change actually works. When I don't test this behaviour change -- then why add tests? On the other hand I'm personally absolutely fine with not testing removing a console.log :) There's really not much that can go wrong

I personally would be fine if the PR didn't have tests in the first place, like you mentioned, it's only a console.log. But since the work is already done testing with a custom renderer, I think it's valuable to preserve that.

If the warning message we're trying to check to never be log never exists in the source code after the PR is merged, then it's very unlikely that test will fail again. If some feature were to be brought back, the warning message will likely be different. If it's the same, it's likely a revert where (as I mentioned above) if we did that there's probably a good reason so and the test would've been removed.

kyr0 · 2024-11-21T23:45:50Z

Moved the example code for a custom renderer / custom frontend framework integration over here: kyr0/astro-custom-renderer

If you plan to advertise your custom renderer example, I would suggest to change the entry points to use a valid extension. Currently they don't have one, which is invalid in Node.js. Every import must have a file extension. In fact, even our container API suggests the use of the extension

The reason why it's actually not that technically "invalid" in this specific case, and worked (even if it looks a bit invalid, I agree!) was that this project uses TypeScript path aliases, as defined in the tsconfig.json file (link).

At transpile/bundle ("compile") time, during development, Vite uses esbuild to transpile TypeScript files and resolve the aliases. For production builds, rollup handles path resolution and optimizations as part of the bundling step, ensuring entry points are correctly adjusted or the code is inlined. Those steps are invisible to the developer (the generated code only exists on-the-fly) when the custom renderer is part of an Astro projects codebase.

Because the code generation is on-the-fly in this case, the change you requested, is not as easy as simply adding a .js, because the .js files for these entrypoints don't exist ahead of time. They are not resolvable at the time of the integration execution. Only the .ts files do exists because there is no bundle step in between. When using path aliases, Vite (as described above) can resolve them and handles transpilation and referencing well - also on-the-fly. Now, in the end, the generated code is valid, when Node.js is executing it. I agree that this is a bit weird for a repo that's meant to be an example.

Changing this (not relying on path aliases) requires a major rework of the example, now with a monorepo structure where the renderer would get its own package, exports, dist build output, and the actual astro example could depend on it. Because Vite would be able to load the built dist files of the module, loading the entrypoints with a .js extension added would work.

It makes sense to implement this structural change, also, because most developers who'd plan to implement their custom renderers or framework integrations wouldn't do it inside of an example Astro project repository. I just thought the example would look way easier without a more complicated monorepo setup. But I agree that in this case, correctness is much more important.

edit:

Refactoring done: https://github.com/kyr0/astro-custom-renderer

Using .js, no path aliases anymore: https://github.com/kyr0/astro-custom-renderer/blob/main/packages/custom-renderer/src/index.ts#L5

kyr0 added 3 commits November 18, 2024 02:30

fix: this message adds no value to developers; it checks against a st…

ba6d58b

…atic list of renderers while Astro allows to dynamically add custom renderers

feat: added framework-custom example to demonstrate the use of custom…

ac506e7

… renderers with the log message fixed

chore: changeset added and package lock updated

a9a5712

github-actions bot added pkg: example Related to an example package (scope) pkg: astro Related to the core `astro` package (scope) labels Nov 18, 2024

chore: fix lint issue, but keeping arguments commented out for refere…

ac1e97d

…nce (real-world use)

kyr0 mentioned this pull request Nov 18, 2024

Implementing Custom Renderers triggers a misleading log message about the renderer not being recognized, while it actually is (and works) #12463

Open

1 task

ematipico requested changes Nov 18, 2024

View reviewed changes

kyr0 added 2 commits November 19, 2024 04:24

chore: removed the example and updated the changeset accordingly

f5569e8

test: added fixture and test to prove the new behaviour

7767450

github-actions bot removed the pkg: example Related to an example package (scope) label Nov 19, 2024

Merge branch 'main' into fix/custom-renderer-warning

839b33f

Merge branch 'main' into fix/custom-renderer-warning

4342263

ematipico approved these changes Nov 20, 2024

View reviewed changes

bluwy reviewed Nov 21, 2024

View reviewed changes

Merge branch 'main' into fix/custom-renderer-warning

b9f2d45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Removes a misleading log message telling that a custom renderer is not recognized while it clearly is. Adding a new example to demo custom renderers for prove. #12461

Fix: Removes a misleading log message telling that a custom renderer is not recognized while it clearly is. Adding a new example to demo custom renderers for prove. #12461

kyr0 commented Nov 18, 2024 •

edited

Loading

changeset-bot bot commented Nov 18, 2024 •

edited

Loading

kyr0 commented Nov 18, 2024

ematipico commented Nov 18, 2024

ematipico left a comment

kyr0 commented Nov 18, 2024 •

edited

Loading

florian-lefebvre commented Nov 18, 2024 •

edited

Loading

kyr0 commented Nov 19, 2024

codspeed-hq bot commented Nov 19, 2024 •

edited

Loading

kyr0 commented Nov 19, 2024 •

edited

Loading

ematipico commented Nov 20, 2024

bluwy Nov 21, 2024

kyr0 Nov 21, 2024 •

edited

Loading

bluwy Nov 22, 2024

kyr0 commented Nov 21, 2024 •

edited

Loading

Fix: Removes a misleading log message telling that a custom renderer is not recognized while it clearly is. Adding a new example to demo custom renderers for prove. #12461

Are you sure you want to change the base?

Fix: Removes a misleading log message telling that a custom renderer is not recognized while it clearly is. Adding a new example to demo custom renderers for prove. #12461

Conversation

kyr0 commented Nov 18, 2024 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Nov 18, 2024 • edited Loading

🦋 Changeset detected

kyr0 commented Nov 18, 2024

ematipico commented Nov 18, 2024

ematipico left a comment

Choose a reason for hiding this comment

kyr0 commented Nov 18, 2024 • edited Loading

florian-lefebvre commented Nov 18, 2024 • edited Loading

kyr0 commented Nov 19, 2024

codspeed-hq bot commented Nov 19, 2024 • edited Loading

CodSpeed Performance Report

Merging #12461 will not alter performance

Summary

kyr0 commented Nov 19, 2024 • edited Loading

ematipico commented Nov 20, 2024

bluwy Nov 21, 2024

Choose a reason for hiding this comment

kyr0 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

bluwy Nov 22, 2024

Choose a reason for hiding this comment

kyr0 commented Nov 21, 2024 • edited Loading

kyr0 commented Nov 18, 2024 •

edited

Loading

changeset-bot bot commented Nov 18, 2024 •

edited

Loading

kyr0 commented Nov 18, 2024 •

edited

Loading

florian-lefebvre commented Nov 18, 2024 •

edited

Loading

codspeed-hq bot commented Nov 19, 2024 •

edited

Loading

kyr0 commented Nov 19, 2024 •

edited

Loading

kyr0 Nov 21, 2024 •

edited

Loading

kyr0 commented Nov 21, 2024 •

edited

Loading