Bugfix 4459 #5586
Bugfix 4459 #5586
Conversation
… the async nature. I think I got it right, but good to check this one).
… in the calling function.
…r that when I duplicated the call to generate its AST. Thus, that introduced a bug. Now fixed.
…oke when lifting the function out of its scope.
…ot getting anything implicitly from the closure).
|
Deploy preview for using-drupal ready! Built with commit fc55e6b |
|
Deploy preview for gatsbygram ready! Built with commit fc55e6b |
…. Bug introduced when I lifted the function out one scope.
…liar with this project and don't know all conventions.
… be parsed in some cases and perhaps not in others.
…the digest hash. Updated the test, but this is risky - could be an unexpected side effect bug?
…what they do with it. So they are really a test of greymatter, not of this code. Updated snaps to show that they don't parse markdown in excerpt or content.
…ult for the digest hash. Updated the test, but this is risky - could be an unexpected side effect bug?" This reverts commit 977dc3d.
|
I reviewed test coverage. There is 0 coverage of the function I modified. It isn't even imported or executed by tests. I tried to add coverage, but the function takes 6 params, many of which are complicated objects. I couldn't figure out how to get either real or fake ones, so I abandoned it. Hopefully a contributor can figure out how to get the params to call this method! |
|
Does someone from the maintainer can please look on this PR? |
KyleAMathews
force-pushed
the
master
branch
5 times, most recently
from
fc4ca3e
to
2116fff
Compare
|
Could we do it other way - ie in here were we set just strip markdown (for example using https://www.npmjs.com/package/remove-markdown)? ---edit: don't mind me I missunderstood this |
* master: (1971 commits) Delete cds-takeaways.md (#8552) chore(release): Publish chore: remove npm-run-all dep and update deps in e2e tests (#8529) [www] Site/Starter Showcase refactor, rinse II (#8411) Rfc process doc (#8537) [www] a11y improvements (#8536) Update the GraphQL stitching blog post & docs (#8516) fix: change teams to reduce noise for people Category typos fixes and consolidation (#8520) debugging build: error due to import/require mix (#8493) updating name Adding pagination docs: add pagination doc (#8476) test: add more unittests for transformer remark (#8272) chore: simplify editorconfig (#8412) chore: format YAML files with Prettier (#8407) feat: add noInlineHighlight option (#7554) chore: bump drupal plugin major version - gatsby v1 version of plugin already uses v2.x.x (#8499) Added option to minify css class names generated in production build for react-css-modules. (#8496) chore: add code owners for www and critical plugins (#8518) ...
|
I've just merged I want to do some more testing on this before merging, which I'm aiming to do later this week. @pieh mentioned that there could be performance impact with generating two AST's, so there may be further updates needed there. |
|
Also thanks to @oorestisime's work (🎉) on the unit tests for |
| } | ||
| const futureContent = processMarkdown(markdownNode.internal.content) | ||
| const futureExcerpt = markdownNode.excerpt | ||
| ? processMarkdown(markdownNode.excerpt) |
There was a problem hiding this comment.
This will process excerpt markdown even if it won't be used - it would be better if this would be more lazy and only process markdown (both for excerpt and content) when someone is querying for it. It would need separate cache keys for content and excerpt ASTs
|
Yes, we can close this one. Thanks for cleanup session @LekoArts! |
Resolves bug #4459 by, when an excerpt is present on the incoming node, parsing it exactly like the node's content.
Implemented this by extracting the functions involved in the parsing from the caching behavior around them, then calling them twice (once for each part). The lowest-level function (which actually does the remark parsing to an AST, asynchronously) is triggered on both parts as soon as the code asks for either. It parses the two in parallel, storing the pair of results in the cache.
The outer functions (
getHTML()and friends) are simply duplicated, with the excerpt HTML stored in the cache just like the content HTML. By duplicated, I mean extracted, parameterized by wrapper, and then the wrapper is duplicated for the other behavior. This minimizes code duplication.Note on my commit messages: each commit is very small. The first character indicates my attempted risk level / kind of transform for that commit. I did make a couple errors along the way; I think I caught them.
The risk level codes I use are:
Finally, I am totally unfamiliar with this code and have no idea how good your test coverage is. I did not extend it. So I believe everything works, but I can't know how confident to be. Some contributor, please help verify!