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

Fix-65575 MarkdownIt plugins (or Rules) called multiple times for one input #65953

Merged
merged 4 commits into from Jan 10, 2019

Conversation

Projects
None yet
2 participants
@skprabhanjan
Copy link
Contributor

skprabhanjan commented Jan 3, 2019

@mjbvz , here is the initial PR based on my understanding to fix #65575 , I have added CachedToken and implementation to use it.
Please review this and let me know if anything is missing or not correct, I will correct it accordingly, Thanks :)

@mjbvz
Copy link
Contributor

mjbvz left a comment

Thanks for taking a look. I think this needs some more work though so that it won't cause regressions:

  • The cache needs to use the document uri as the key for storing the tokens. One way to do this would be to create a new private method called tokenize or something like that that both render and parse call into. The tokenize method would check the cached tokens to see if they are valid for the current document. If they are, it would use these. Otherwise, it would compute and cache the tokens for the new document

  • However there is also a small difference in render and parse: parse always strips out markdown yaml front-matter while render only strips the front-matter if a setting is configured. One way to preserve this behavior would be to run markdown-its render on the tokens without the front matter and then add the front matter back on to the html output

Let me know if that makes sense

@skprabhanjan

This comment has been minimized.

Copy link
Contributor Author

skprabhanjan commented Jan 4, 2019

@mjbvz , Cool , this sounds good and makes sense , I am working on it and update you on the progress. Thanks for the review :)

pkoushik
@skprabhanjan

This comment has been minimized.

Copy link
Contributor Author

skprabhanjan commented Jan 4, 2019

@mjbvz , I have changed it as per your review , Please review it now and let me know if this is good or something needs to be changed , Happy to correct it (Just want to make sure it is good).
Thanks :)

@mjbvz
Copy link
Contributor

mjbvz left a comment

Looks light the right direction. Please take a look my comments and let me know if they make sense

pkoushik

@mjbvz mjbvz merged commit 9140285 into Microsoft:master Jan 10, 2019

1 of 2 checks passed

VS Code #20190108.22 failed
Details
license/cla All CLA requirements met.
Details

@mjbvz mjbvz added this to the December/January 2019 milestone Jan 10, 2019

@mjbvz

This comment has been minimized.

Copy link
Contributor

mjbvz commented Jan 10, 2019

Thanks! Will make a few tweaks to this but approach looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment