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

feat(gatsby-transformer-remark): fall back to pruneLength if no content separator present #19137

Merged
merged 14 commits into from
Dec 10, 2019

Conversation

samrae7
Copy link
Contributor

@samrae7 samrae7 commented Oct 30, 2019

Description

  • Up until now when gatsby-transformer-remark is configured to use a content separator, but there is not a content separator present in the markdown file, when a query for the excerpt is made with format: MARKDOWN specified, the excerpt for that node is returned as an empty string, even when pruneLength param is sent with the query. See issue: excerpt needs to fall back to pruneLength when no excerpt_separator #18312).

  • This change will mean that in the above case the excerpt is returned based on the pruneLength param passed in the query. This means that there is a fall back behaviour when separators are not present.

  • As part of this change I implemented this TODO:

    // TODO truncate respecting markdown AST

    (using MarksdownAst to truncate the excerpt). This meant some refactoring to getExcerptASt and getExcerptHtml methods in the same file

Related Issues

fixes #18312

@samrae7 samrae7 requested review from a team as code owners October 30, 2019 08:45
@samrae7 samrae7 changed the title Topics/issue18312 Markdown excerpt: fall back to pruneLength if no content separator Topics/issue18312: Markdown excerpt query falls back to pruneLength if no content separator present Oct 30, 2019
@samrae7 samrae7 changed the title Topics/issue18312: Markdown excerpt query falls back to pruneLength if no content separator present feat(gatsby-transformer-remark): fall back to pruneLength if no content separator present(addresses #18312) Oct 30, 2019
length: pruneLength,
omission: `…`,
const ast = await getMarkdownAST(markdownNode)
const excerptAST = await getExcerptAst(ast, markdownNode, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your changes getExcerptAst expect first param to be html AST (not markdown AST) (still trying to wrap my head around why this is needed). Is this correct here?

Copy link
Contributor Author

@samrae7 samrae7 Nov 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pieh thanks for the review. My understanding is that getExcerptAst is a function that will take either kind of AST(html or markdown)

Based on the pruneLength it identifies the last node that should be included. It prunes or truncates that lastNode so that you have an AST that just represents the excerpt. If there is a content separator it will instead use that to generate the tree, and if the pruneLength is less than the length of the markdown it will just return the full tree.

It is being used by getExcerptHtml to get the excerpt when format: HTML is specified so I started using it for the markdown case. An alternative would be to simply prune/truncate the rawMarkdownBody (which is what it was doing before: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-transformer-remark/src/extend-node-type.js#L422). In that case my change would be simpler and would just be adding a check to this line: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-transformer-remark/src/extend-node-type.js#L418 to check that markdown.excerpt ! == ""

I thought that the reason the html method doesn't do this is that it could end up returning strings where the markdown symbols have been truncated ( ie. you could end up with a sentence cut off with only part of some markdown like Hello **World** being retuned as Hello **World*). I am not sure about this though ( do you know?). I could do some testing to find out.

Did I address your question? I will test the above when I get time ( will be next day or so). In the meantime if you have context on it please let me know.

Copy link
Contributor Author

@samrae7 samrae7 Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pieh I've added a test that shows what I'm talking about ( I hope). The latest test I've added wouldn't pass if we didn't use the AST to generate the pruned excerpt. It would count markdown characters in the pruneLength and would return cut-off markdown in some cases. ie. in the test case I've added it would return Where oh where **is… as opposed to Where oh where **is**…

Does this answer your question or were you asking something different?

Copy link
Contributor Author

@samrae7 samrae7 Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pieh could you take a look at my reply above? Also, if I misunderstood your question please let me know. Thanks. I just re read your question and the short answer is that either kind of AST will be transformed into an excerptAst by that method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pieh ^

@LekoArts LekoArts added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Dec 2, 2019
Copy link

@fabianrios fabianrios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test created is enough to solve the questions raised for this topic, plus it solve the required fallback behaviour

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in, thanks @samrae7!

@pieh pieh changed the title feat(gatsby-transformer-remark): fall back to pruneLength if no content separator present(addresses #18312) feat(gatsby-transformer-remark): fall back to pruneLength if no content separator present Dec 10, 2019
@pieh pieh merged commit fceb790 into gatsbyjs:master Dec 10, 2019
@pieh
Copy link
Contributor

pieh commented Dec 10, 2019

Published in gatsby-transformer-remark@2.6.42

@samrae7 samrae7 deleted the topics/issue18312 branch February 2, 2020 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

excerpt needs to fall back to pruneLength when no excerpt_separator
4 participants