analogjs / analog

The fullstack meta-framework for Angular. Powered by Vite and Nitro
https://analogjs.org
MIT License
2.5k stars 240 forks source link

injectContentFiles does not provide the actual content: marked(): input parameter is undefined or null #981

Closed jvoigt closed 3 months ago

jvoigt commented 5 months ago

Please provide the environment you discovered this bug in.

I created an minimal examble here: https://github.com/jvoigt/bug-analog-inject-content-files/blob/main/src/app/pages/index.page.ts#L26

Somehow i did not get a stackblitz for it to run.

I tried it on

Which area/package is the issue in?

content

Description

Thanks tor all the great work. So far i'm realy loving analogjs and am planing to use it in my next project.

I noticed that both injectContent and injectContentFiles return a ContentFile. Only injectContent doing it as Observable. But to me it looks like, injectContentFiles does not actually fetch the markdown – the type say it does though. The docs dont mention this. So i would guess the behavoir is actualy working as intended, its just the types and docs that could be more precise.

Please provide the exception or error you saw

So when trying to render the markdown with MarkdownComponent markedjs will produce an Error:

ERROR Error: marked(): input parameter is undefined or null
Please report this to https://github.com/markedjs/marked.
    at Marked.parse (@analogjs_content.js?v=5ac38bec:6623:25)
    at Function.marked [as parse] (@analogjs_content.js?v=5ac38bec:6741:25)
    at _MarkdownContentRendererService.render (@analogjs_content.js?v=5ac38bec:8101:60)
    at _AnalogMarkdownComponent.renderContent (@analogjs_content.js?v=5ac38bec:8223:33)
    at @analogjs_content.js?v=5ac38bec:8220:130
    at doInnerSub (chunk-6QJEESQ7.js?v=5ac38bec:2222:15)
    at outerNext (chunk-6QJEESQ7.js?v=5ac38bec:2216:34)
    at OperatorSubscriber2._this._next (chunk-6QJEESQ7.js?v=5ac38bec:874:9)`

image

below i tried the same with injectContent and it works

Other information

I would guess that the fetching of the markdown is the reason the injectContent is async.. But the types does not reflect that.

i would be willing to help with fixing that but maybe would require some time and guidance, as i just started experimenting with analog.

I would be willing to submit a PR to fix this issue

joshuamorony commented 5 months ago

I think what is likely happening here is that content is optional on the type, but in the analog-markdown component the fallback behaviour for content being undefined is to use data defined on the ActivatedRoute which will not exist, and so it causes the error.

If analog-markdown is not handling the optional case properly then yes the solution here is probably to restrict the type of injectContentFiles more so that it omits content entirely.

I've made some assumptions here that might not be entirely accurate, but maybe that can at least help direct your efforts if you want to work on a PR

jvoigt commented 3 months ago

hey, sadly i'm quiet busy at the moment... so sadly i wont find time for a pr until some what in the late summer. :(

so if someone wants to pick this up: please do so!

stewones commented 3 months ago

going through the same. I need to list all blog posts on a page, but each post must have the content rendered already. like a giant page containing all the posts in full.

brandonroberts commented 3 months ago

Interesting use case. We figured most would want only the frontmatter for list pages. You can use a separate glob function to get all the files manually if needed.

const allPosts = import.meta.glob(['/src/content/**/*.md'], { eager: true }); // Record<string, { default: string }>

You can parse the files as you wish from there

Docs: https://vitejs.dev/guide/features.html#glob-import

stewones commented 3 months ago

Interesting use case. We figured most would want only the frontmatter for list pages. You can use a separate glob function to get all the files manually if needed.

const allPosts = import.meta.glob(['/src/content/**/*.md'], { eager: true }); // Record<string, { default: string }>

You can parse the files as you wish from there

Docs: https://vitejs.dev/guide/features.html#glob-import

yeah but then I'd have to deal with extracting frontmatter, parsing content, markdown etc, all the stuff Analog does for us right?

I was thinking about adding support for this use case using injectContent which seems to be almost there with the current API

this way we could just:

 injectContentFiles<PostAttributes>()
    .map((contentFile) => {
      const parts = contentFile.filename.split('/');
      const filename = parts[4];
      const subdirectory = parts[3];

      const content = toSignal(
        injectContent({
          filename,
          subdirectory,
        }),
      );

      return content();
    })

I can put it all together in a PR if you think that would make sense @brandonroberts

stewones commented 3 months ago

so I got this working locally, the implementation is a little different than I previously thought, but it works great with minimal changes to the injectContent API to support this new option filename:

// resolver which returns an observable
export function injectBlogPosts() {
  const contentFiles = injectContentFiles<PostAttributes>();
  const contentStream = contentFiles.map((contentFile) => {
    const parts = contentFile.filename.split('/');
    const filename = parts[4];
    const subdirectory = parts[3];
    return injectContent({
      filename,
      subdirectory,
    });
  });

  return forkJoin(contentStream);
}
// the component consuming it
export default class BlogsComponent {
  posts = toSignal(injectBlogPosts());
}
@for (post of posts()) {
 <h1>{{post.attributes.title}}</h1>
 <p>{{post.content}}</p>         
} 
brandonroberts commented 3 months ago

If its just supporting a custom filename, injectContent already supports this.

injectContent({ customFilename: 'path-to-file' })

https://github.com/analogjs/analog/blob/beta/packages/content/src/lib/content.ts#L87

stewones commented 3 months ago

the problem is that customFilename doesn't support subdirectories. my implementation would add this new combo option:

Screenshot 2024-05-22 at 13 45 33

this way we can have a content structure like this:

Screenshot 2024-05-22 at 13 47 52

brandonroberts commented 3 months ago

It does, as customFilename lets you provide the whole path.

export function injectBlogPosts() {
  const contentFiles = injectContentFiles<PostAttributes>();
  const contentStream = contentFiles.map((contentFile) => {
    const parts = contentFile.filename.split('/');
    const filename = parts[4];
    const subdirectory = parts[3];
    return injectContent({
      customFilename `${subdirectory}/${filename}` // /blog/2014-12-23-...
    });
  });

  return forkJoin(contentStream);
}
stewones commented 3 months ago

didn't realise customFilename was supposed to work like that too, because it's not working 😅

I'll try to figure out where it's broken, but one thing I can confirm is that the injectBlogPosts implementation above won't work because the observable inside getContentFile never completes, it's only emitting in the current version.

this is one of the changes I'd do:

Screenshot 2024-05-22 at 14 14 29

brandonroberts commented 3 months ago

That's a reasonable change that can be done to enable this though

stewones commented 3 months ago

That's a reasonable change that can be done to enable this though

nice I'll figure it out and send a PR.

out of curiosity: why do we need an observable here if it's supposed to emit only once?

brandonroberts commented 3 months ago

That's a reasonable change that can be done to enable this though

nice I'll figure it out and send a PR.

out of curiosity: why do we need an observable here if it's supposed to emit only once?

The short answer is zone.js 😄