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

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

Closed
1 of 2 tasks
jvoigt opened this issue Mar 29, 2024 · 13 comments · Fixed by #1110
Closed
1 of 2 tasks
Labels
bug Something isn't working

Comments

@jvoigt
Copy link

jvoigt commented Mar 29, 2024

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

  • node v20.11.0
  • npm 10.2.4
  • @analogjs/content 1.0.2

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

  • Yes
  • No
@jvoigt jvoigt added the bug Something isn't working label Mar 29, 2024
@joshuamorony
Copy link
Contributor

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
Copy link
Author

jvoigt commented May 18, 2024

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
Copy link
Contributor

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
Copy link
Member

brandonroberts commented May 21, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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

@stewones
Copy link
Contributor

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
Copy link
Member

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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants