withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
46.93k stars 2.49k forks source link

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

Open kyr0 opened 4 days ago

kyr0 commented 4 days ago

Changes

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:

Bildschirmfoto 2024-11-18 um 02 38 12

After:

Bildschirmfoto 2024-11-18 um 02 35 02

Fixes: https://github.com/withastro/astro/issues/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.

changeset-bot[bot] commented 4 days ago

🦋 Changeset detected

Latest commit: b9f2d45646b3ac72d94357f1e58d45d0245d0b35

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 commented 4 days ago

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

Bildschirmfoto 2024-11-18 um 04 05 36
ematipico commented 4 days ago

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.

kyr0 commented 3 days ago

@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 commented 3 days ago

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

kyr0 commented 3 days ago

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

codspeed-hq[bot] commented 3 days ago

CodSpeed Performance Report

Merging #12461 will not alter performance

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

Summary

✅ 6 untouched benchmarks

kyr0 commented 3 days ago
Bildschirmfoto 2024-11-19 um 06 17 04

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 commented 2 days ago

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

kyr0 commented 13 hours ago

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