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): add excerptAst to be exported as a GraphQL field #11237

Merged
merged 7 commits into from Feb 25, 2019

Conversation

Projects
None yet
2 participants
@chitoku-k
Copy link
Contributor

chitoku-k commented Jan 23, 2019

Description

This PR adds/refactors excerpt + excerptAst (which is a new field) because I personally found it incovenient gatsby-transformer-remark not exporting an excerpt field in AST. What I did was basically refactoring by splitting out the resolver of excerpt that was internally converting ASTs to HTML to two functions that have similar signatures to the getHtml() and getHtmlAst(), with some caching feature as well. The tests have been added for these ASTs.

Documentation could be added something like this:

diff --git a/packages/gatsby-transformer-remark/README.md b/packages/gatsby-transformer-remark/README.md
index 423329b..36a0d59 100644
--- a/packages/gatsby-transformer-remark/README.md
+++ b/packages/gatsby-transformer-remark/README.md
@@ -119,6 +119,7 @@ By default, excerpts have a maximum length of 140 characters. You can change the
       node {
         html
         excerpt(pruneLength: 500)
+        excerptAst(pruneLength: 500)
       }
     }
   }
@@ -178,14 +181,15 @@ Any file that does not have the given `excerpt_separator` will fall back to the

 ### Excerpts for non-latin languages

-By default, `excerpt` uses `underscore.string/prune` which doesn't handle non-latin characters ([https://github.com/epeli/underscore.string/issues/418](https://github.com/epeli/underscore.string/issues/418)).
+By default, `excerpt` and `excerptAst` use `underscore.string/prune` which doesn't handle non-latin characters ([https://github.com/epeli/underscore.string/issues/418](https://github.com/epeli/underscore.string/issues/418)).

-If that is the case, you can set `truncate` option on `excerpt` field, like:
+If that is the case, you can set `truncate` option on `excerpt` and `excerptAst` field, like:

 ```graphql
 {
   markdownRemark {
     excerpt(truncate: true)
+    excerptAst(truncate: true)
   }
 }

@chitoku-k chitoku-k changed the title chore(gatsby-transformer-remark): add excerptAst to be exported as a GraphQL field fix(gatsby-transformer-remark): add excerptAst to be exported as a GraphQL field Jan 23, 2019

@chitoku-k chitoku-k force-pushed the chitoku-k:feature/transformer-remark branch from f1c15f4 to 2aa0c3a Jan 23, 2019

@chitoku-k

This comment has been minimized.

Copy link
Contributor Author

chitoku-k commented Jan 24, 2019

@DSchau Could you help me find why this test sub-plugin caching fails while the others pass? I dug into an actual problem that caused the test to fail but got no luck with it. Thanks in advance.

@chitoku-k

This comment has been minimized.

Copy link
Contributor Author

chitoku-k commented Jan 24, 2019

I have finally figured out that the test added in #10892 did not handle Promises that are passed to visit. Each Promise must be wrapped by Promise.all() so that the gatsby-transformer-remark knows the sub-plugins finished their procedures; otherwise, it can easily break with such an unrelated change.

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Jan 24, 2019

@chitoku-k so this PR seems to make sense in general. If we have htmlAST we should also expose an excerptAST similarly.

However - if you don't mind me asking, apart from being consistent here, what is this PR solving?

@chitoku-k

This comment has been minimized.

Copy link
Contributor Author

chitoku-k commented Jan 24, 2019

@DSchau My project currently tries to utilize gatsby-remark-component that requires the AST be passed to its compiler. It works fine so far with htmlAsts, but the excerpts had to be manually generated using the same logic. This PR solves this problem rather than making consistency here.
Thanks.

@chitoku-k chitoku-k force-pushed the chitoku-k:feature/transformer-remark branch from de6c49b to 1295a30 Jan 29, 2019

@chitoku-k chitoku-k requested a review from pieh Jan 29, 2019

@DSchau
Copy link
Contributor

DSchau left a comment

This looks good! Left a small comment.

Nice catch on the async issue, as well!

`transformer-remark-markdown-excerpt-${format}-${
node.internal.contentDigest
}-${pluginsCacheStr}-${pathPrefixCacheStr}`
const excerptAstCacheKey = node =>

This comment has been minimized.

@DSchau

DSchau Jan 29, 2019

Contributor

Couldn't the excerptCacheKey be used with a format? Instead of two of the same function basically.

This comment has been minimized.

@chitoku-k

chitoku-k Jan 29, 2019

Author Contributor

Yes, does it look better?

@chitoku-k

This comment has been minimized.

Copy link
Contributor Author

chitoku-k commented Feb 21, 2019

@DSchau Any further thoughts for this?

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Feb 21, 2019

@chitoku-k checking it out now. Thanks for the reminder!

@DSchau DSchau changed the title fix(gatsby-transformer-remark): add excerptAst to be exported as a GraphQL field feat(gatsby-transformer-remark): add excerptAst to be exported as a GraphQL field Feb 21, 2019

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Feb 21, 2019

I took a look! It looks like the arguments passed to excerpt (e.g. pruneLength) do not seem to be taking effect.

For instance, if we consider the following query, we'd expect the excerpt to be 1 character, as well as the excerpt provided in the AST should be 5 characters.

{
  allMarkdownRemark(limit:1) {
    edges {
      node {
        excerpt(pruneLength:1)
        excerptAst(pruneLength:5)
      }
    }
  }
}

This, unfortunately does not seem to be the case! I can dive in in a bit, but I don't believe this to be in a mergeable state quite yet. Would you be able to validate on your end that you're seeing the same?

For the record - I used gatsbyjs.org to test!

For posterity, here's the result of the above GraphQL query.

@chitoku-k

This comment has been minimized.

Copy link
Contributor Author

chitoku-k commented Feb 23, 2019

@DSchau Thank you for reviewing and detailed description for the bug!
It turned out that the parameterized fields can never been cached without parameterized cache keys, as any later changes would no longer take effect. There should be chances that these parameters can be included to cache key, but I removed them because it didn't sound good choice to me.

chitoku-k added some commits Jan 23, 2019

@chitoku-k chitoku-k force-pushed the chitoku-k:feature/transformer-remark branch from a5b45fd to c29290f Feb 23, 2019

@chitoku-k

This comment has been minimized.

Copy link
Contributor Author

chitoku-k commented Feb 23, 2019

Fixed the regression and rebased to master (due to failures on e2e tests).

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Feb 25, 2019

@chitoku-k thank you! I'll take one more look at this, and then hopefully get it merged and published today. Thank you for your patience, and for fixing the issues as they arose!

@DSchau

DSchau approved these changes Feb 25, 2019

Copy link
Contributor

DSchau left a comment

Doing some final validations now, but I think this looks good. Will merge in a few unless I find anything!

Thanks!

@DSchau DSchau merged commit e59d4ca into gatsbyjs:master Feb 25, 2019

14 checks passed

Peril All green. Congrats.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsbygram Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node6 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
@chitoku-k

This comment has been minimized.

Copy link
Contributor Author

chitoku-k commented Feb 26, 2019

I appreciate all your reviews! Thanks.

@chitoku-k chitoku-k deleted the chitoku-k:feature/transformer-remark branch Feb 26, 2019

DSchau added a commit that referenced this pull request Mar 19, 2019

fix(gatsby-transformer-remark): always include the root node of AST (#…
…12647)

## Description

Fixes a bug in `cloneTreeUntil` function in `gatsby-transformer-remark`, where it only returns the last child node. This function is expected to clone a `root` node, but the current implementation forgets to revert the variable back to the `root` node. Reassigning `clonedRoot = newNode` (originally this is the argument `root`) should fix this. By resolving this bug, it appears to correctly returns the entire root node of AST in `excerptAst` of the markdown node, instead of returning the partial result.

## Related Issues

Related to #11237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.