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

[WIP] Finish initial SSR (server side rendering) and add tests. #1227

Draft
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

trusktr
Copy link
Member

@trusktr trusktr commented Jun 17, 2020

Summary

Moved from anikethsaha#3

Expands on stuff from #1128 (this PR includes those changes) which fixes #1126.

This will PR do the following (WIP):

  • replaces custom URL analysis (custom regex) with DOM APIs that are known to work in isExternal

  • adds tests for it

  • fixes SSR because isExternal is used there, and to test it there we must fix it (one line fix)

  • adds tests for SSR now that it is fixed

  • uses DOM APIs (JSDOM in Node.js and SSR, otherwise native APIs) for the isExternal function

  • consolidate isExternal so all code imports the same version

  • initial tests for server-renderer (WIP, we can't test DOMPurify in SSR if SSR isn't working, so first we get SSR working and add a simple test for it)

  • add tests for isExternal and DOMPurify (WIP)

  • cleanup (remove comments, use conventional commit messages, etc)

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

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

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

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

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

#1126

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

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

@vercel
Copy link

vercel bot commented Jun 17, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/2CFnWQmsbQ8UvXud3MuYLjBhAyDS
✅ Preview: https://docsify-preview-git-fix-validating-remote-c-cfafef-docsify-core.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 17, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6be2965:

Sandbox Source
confident-ptolemy-9qr4s Configuration

expect(isExternal('//google.com/foo.md')).to.be.true;
expect(isExternal('/google.com/foo.md')).to.be.false;
});
});
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't get to circle back to this yet, but it is high on my priority list. These tests are the important part, so we know it is working well.

@trusktr trusktr force-pushed the fix-validating-remote-content-2 branch from e5d8ecc to bb1e84a Compare July 5, 2020 01:04
@trusktr trusktr force-pushed the fix-validating-remote-content-2 branch from bb1e84a to 6be2965 Compare July 5, 2020 01:38
@trusktr trusktr changed the title Fix validating remote content 2 improve validating remote content, fix SSR and test it Jul 7, 2020
@trusktr trusktr mentioned this pull request Jul 16, 2020
18 tasks

await renderer.renderToString('/changelog');

expect(renderer).to.be.an.instanceof(Renderer);
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO, server tests here

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.

is this independent of #1276 ?

Also, if its still in WIP, can you convert this into draft.

@sy-records sy-records linked an issue Nov 1, 2020 that may be closed by this pull request
@trusktr trusktr mentioned this pull request Nov 1, 2020
* develop: (81 commits)
  fix: upgrade dompurify from 2.1.0 to 2.1.1 (#1402)
  fix: upgrade dompurify from 2.0.17 to 2.1.0 (#1397)
  fix: search on homepage test (#1398)
  fix: the sidebar links to another site. (#1336)
  fix: Can't search homepage content (#1391)
  fix: upgrade debug from 4.1.1 to 4.3.0 (#1390)
  fix: packages/docsify-server-renderer/package.json & packages/docsify-server-renderer/package-lock.json to reduce vulnerabilities (#1389)
  Fix eslint warnings (#1388)
  docs: add crossOriginLinks configurations details. (#1386)
  Remove Cypress screenshots
  Fix friendly message display
  Add Vue 3 compatibility
  Show dir listing & help msg for manual instance
  Add NODE_MODULES_URL global
  Jest + Playwright Testing (#1276)
  update doc (#1381)
  Fix scroll event end value
  fix: upgrade docsify from 4.11.4 to 4.11.6 (#1373)
  chore(deps): bump node-fetch in /packages/docsify-server-renderer (#1370)
  test: fix cannot search list content (#1367)
  ...
@gpetrov
Copy link

gpetrov commented Jan 13, 2021

any progress on this? We would really like to get SSR working

@dnlup dnlup mentioned this pull request Apr 1, 2021
4 tasks
…to specifically use .cjs (those tools don't support Node ESM files yet), use a single eslintrc file, all test code uses ESM import/export syntax instead of require (WIP)
* develop: (104 commits)
  chore: bump ssri from 6.0.1 to 6.0.2 (#1563)
  chore: Update Edit Document using develop branch (#1541)
  fix: Add escapeHtml for search (#1551)
  docs: link with plugin Pagination (#1554)
  fix: Upgrade dompurify from 2.2.6 to 2.2.7 (#1553)
  fix: upgrade dompurify from 2.2.6 to 2.2.7 (#1552)
  chore: bump y18n from 4.0.0 to 4.0.1 (#1548)
  chore: Fix search for missing pathNamespaces (#1547)
  fix: Upgrade docsify from 4.12.0 to 4.12.1 (#1544)
  docs:Update deploy, change Zeit to Vercel (#1540)
  fix: Cannot read property 'classList' of null (#1527)
  chore: fix microsoft/playwright-github-action error (#1534)
  Update PULL_REQUEST_TEMPLATE.md
  chore: Update CHANGELOG and Update test snapshots
  chore: add changelog 4.12.1
  [build] 4.12.1
  feat: Support search when there is no title (#1519)
  test(unit): add test cases on isExternal. (#1515)
  docs: Update Vercel logo link (#1520)
  fix: Upgrade docsify from 4.11.6 to 4.12.0 (#1518)
  ...
…erly fail so much, and log errors to console instead of swallowing them
Many things work. What doesn't work (needs work):

- Any dynamic config options (f.e. any that use functions) need to be specified in the HTML by augmenting the injected config object. This includes plugins, Vue components, and similar.
- Plugins and Vue don't work on initial SSR render. Switch pages dynamically and they work during the dynamic markdown handling.
- Cover page causes an error
- Search plugin causes an error
@trusktr
Copy link
Member Author

trusktr commented May 18, 2021

I just pushed a working concept! The commit describes a few things that don't work, but for basic without plugins or Vue components it will work.

Up next we need to figure how to handle plugins, and Vue components (@jhildenbiddle).

We need to call some parts of plugins during SSR markdown handling, and other parts of plugins on the client side where they can control the resulting DOM.

Search and cover page aren't working (they have some URL mis-handling, similar to what I fixed in this PR in other areas of the code, just needs some more work. Next time!).

@trusktr trusktr changed the title improve validating remote content, fix SSR and test it [WIP] Finish initial SSR (server side rendering) and add tests. May 21, 2021
@trusktr
Copy link
Member Author

trusktr commented May 21, 2021

I made an update to the title to better reflect what this has become (finishing the initial SSR implementation). A side-effect of it is adding tests for some features (that was the original intent of the PR).

@trusktr trusktr added enhancement semver-minor This needs a semver-minor release vuejs related to Vue.js WIP labels May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement semver-minor This needs a semver-minor release vuejs related to Vue.js WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The SSR example failed Cannot read property 'indexOf' of undefined vulnerability report
3 participants