Skip to content

fix #970, updated the insertion of embedded tokens for async returns #1102

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

Merged
merged 1 commit into from
May 22, 2020
Merged

fix #970, updated the insertion of embedded tokens for async returns #1102

merged 1 commit into from
May 22, 2020

Conversation

tschaible
Copy link

@tschaible tschaible commented Apr 2, 2020

Summary

fix #970 - updated the insertion of embedded tokens so it correctly handles embedded token results returning in any order

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes

  • No

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

Other information:

The previous implementation incorrectly assumed that contents of embedded content would be returned in the order they were requested in the document. This created unpredictable race conditions when multiple embeds were included.

I have run into this issue with embedded markdown, and have only attempted test-cases surrounding the markdown content specifically.


  • [X ] DO NOT include files inside lib directory.

…dded token results returning in any order

fixes #970
@tschaible
Copy link
Author

When I initially opened this, 2 of the e2e test phases were marked as failed.

I was able to investigate this morning. The only errors were timeouts in the before phases of the test suite.

I ran the e2e tests locally without issue and have re-pushed the change. This time everything passed without issue.

I believe the original failure was simply an intermittent failure of the test framework setup.

I have also done some basic testing with Firefox and Safari, in addition to my additional tests in Chrome. I am not planning further updates, at this time. This change seems to effectively fix #970 in my use cases.

Please let me know if there is anything else I can do to assist in getting this merged.

@anikethsaha
Copy link
Member

thanks for the PR and sorry for the delayed.
the tests are not so strong as of now, it bugs most of the time.
I will try to review it soon .

@anikethsaha
Copy link
Member

cc @docsifyjs/reviewers

@anikethsaha anikethsaha requested review from anikethsaha, a team, trusktr and Koooooo-7 and removed request for a team April 7, 2020 13:02
Copy link
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

I test all the embed file cases we supported, it works fine. 💯

@anikethsaha
Copy link
Member

@Koooooo-7 can you approved the PR if its looks fine by you. I am in the process of adding 2 required approval before merging.

@erbridge
Copy link

erbridge commented May 3, 2020

This is affecting us, too. Any chance it could be merged?

Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

Looks good, minor suggestion

@anikethsaha anikethsaha added the semver-patch This needs a patch release label May 18, 2020
@anikethsaha anikethsaha merged commit 5ea28c4 into docsifyjs:develop May 22, 2020
githoniel added a commit to gem-mine/docsify that referenced this pull request Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch This needs a patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple embed files not loaded correctly
4 participants