WIP, updating server.js to use JSDOM to fix isExternal, but also some… #3

trusktr · 2020-05-22T03:09:42Z

This is a "draft" pull request, for discussion. Let's figure out what we want to do.

trusktr · 2020-05-22T03:10:31Z

build/ssr.js

+      //     transforms: {
+      //       generator: false,
+      //     },
+      //   }),


The output is nice without this. async functions and generators work great in newer Node.

trusktr · 2020-05-22T03:11:13Z

build/ssr.js


-    console.log(dest)
+    console.log(dest);


auto formatting based on the repo's eslint/prettier config files.

trusktr · 2020-05-22T03:11:56Z

package.json

@@ -48,7 +48,7 @@
    "pub": "sh build/release.sh",
    "postinstall": "opencollective-postinstall"
  },
-  "husky": {
+  "husky-OFF": {


temporary. if we accept any changes, they should all be formatted.

Actually we should just go ahead and format the whole code base all in one PR and get it out of the way if we're going to have the eslint/prettier configs in the repo.

trusktr · 2020-05-22T03:12:32Z

packages/docsify-server-renderer/index.js


 function cwd(...args) {
  return resolve(process.cwd(), ...args);
 }

 // Borrowed from https://j11y.io/snippets/getting-a-fully-qualified-url.
 function qualifyURL(url) {
+  // TODO this doesn't work in Node, passing in / results in /. It doesn't know the origin. Maybe we should update `location` globally first.


nevermind this comment. I fixed it by configuring JSDom.

trusktr · 2020-05-22T03:12:49Z

packages/docsify-server-renderer/index.js

    this.html = template;
    this.config = config = Object.assign({}, config, {
      routerMode: 'history',
    });
-    this.cache = cache;
+
+    // this.cache = cache; // not used for anything?


Doesn't seem to be used for anything.

trusktr · 2020-05-22T03:13:27Z

server.js

-    <!--inject-config-->
-  <script src="/lib/docsify.js"></script>
-  </body>
-  </html>`,


moved to a separate HTML file so it can be re-used (f.e. here, and in the test code).

trusktr · 2020-05-22T03:14:12Z

server.js

    },
-    path: './'
-  })
+    // path: './', // not used for anything?


Unused? The Renderer constructor doesn't use this option for anything, unless I missed it.

Actually, I found out because I add // @ts-check at the top of any JS file and have TypeScript do live type checking in VS Code, and it complained that the option was unused (which appears to be the case).

Looks like we have some cleanup to do.

trusktr · 2020-05-22T03:14:58Z

test/unit/server.test.js

+      // path: './', // not used for anything?
+    });
+
+    expect(renderer).to.be.an.instanceof(Renderer);


A starting point to officially test the server code.

trusktr · 2020-05-22T03:15:51Z

server.js

+  const { initJSDOM } = require('./test/_helper');
+
+  const dom = initJSDOM();
+  dom.reconfigure({ url: 'https://127.0.0.1:3000' });


This is the most important change. This makes isExternal work.

Renderer already needs to use DOM APIs anyways, so it's not a big deal to set up JSDom here. I'm not sure how the DOM APIs were being set up before though... Unless this server.js never worked? What is it used for?

trusktr · 2020-05-22T03:18:03Z

server.js

+  const {
+    Renderer,
+    getDefaultTemplate,
+  } = require('./packages/docsify-server-renderer/index');


Import the ES Modules. Much nicer experience in the debugger (Chrome devtools).

Try it:

npm i -g ndb SSR=true ndb server.js

Once you run it, you should see an error inside the compiler.path() method. Pause devtools on uncaught exceptions in the Sources tab.

trusktr · 2020-05-22T03:19:24Z

server.js

+    getDefaultTemplate,
+  } = require('./packages/docsify-server-renderer/index');
+
+  debugger;


temporary. I will delete this commit and recreate the branch all cleaned up. We just need to decide what to do.

anikethsaha · 2020-05-22T06:00:32Z

I didnt give a deep look but it looks great.

I will check this locally.

we can support node v10 as a minimum.

also, for this docsifyjs#1128, I was thinking we can create a separate PR with your implement just to reduce the complexity? It will help you as well to work in that better.

we can open the PR just for the reference.

what do you think?

trusktr · 2020-05-23T00:23:39Z

Sure. 👍

Probably we should open any new branches in the main repo, that way if we do pull requests onto each others' branches we can view them all in the main repo and other people can chime in too (instead of opening PRs in my or your personal repos).

anikethsaha · 2020-05-23T05:43:40Z

great 👍

This takes a string input, and converts it into a fully-qualified URL. For example, `./foo` becomes `http://example.com/current/path/foo`. Then we can compare it to other fully-qualified `URL` instances.

…rk. Need to make a test to test that DOMPurify is working, and also isExternal is working using <img> API

trusktr · 2020-06-17T09:37:12Z

Moving this to a PR in the main repo for visibility; docsifyjs#1227

trusktr commented May 22, 2020

View reviewed changes

build/ssr.js

console.log(dest)

console.log(dest);

Copy link

Author

trusktr May 22, 2020

Choose a reason for hiding this comment

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

auto formatting based on the repo's eslint/prettier config files.

trusktr commented May 22, 2020

View reviewed changes

anikethsaha force-pushed the fix-validating-remote-content branch from 45023a4 to 1e3dd13 Compare June 14, 2020 07:12

trusktr added 6 commits June 16, 2020 23:41

feat: Qualify URLs then see if the origins match.

0a8deed

This takes a string input, and converts it into a fully-qualified URL. For example, `./foo` becomes `http://example.com/current/path/foo`. Then we can compare it to other fully-qualified `URL` instances.

feat: also update the client isExternal check

ed08428

fix: fix lint errors

bbeaafd

fix: automatically fix some lint warnings

8dd273b

use the more robust and shared isExternal function

2bc8253

add an intial test file for testing SSR

22f1b2b

trusktr force-pushed the tmp-update-server branch from 20e735a to 329120f Compare June 17, 2020 06:51

WIP compiler.parse() is called without arg, causes SSR mode to not wo…

f18b994

…rk. Need to make a test to test that DOMPurify is working, and also isExternal is working using <img> API

trusktr force-pushed the tmp-update-server branch from 329120f to f18b994 Compare June 17, 2020 07:57

trusktr closed this Jun 17, 2020

trusktr mentioned this pull request Jun 19, 2020

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

Draft

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP, updating server.js to use JSDOM to fix isExternal, but also some… #3

WIP, updating server.js to use JSDOM to fix isExternal, but also some… #3

trusktr commented May 22, 2020

trusktr May 22, 2020

trusktr May 22, 2020

trusktr May 22, 2020

trusktr May 22, 2020

trusktr May 22, 2020

trusktr May 22, 2020 •

edited

Loading

trusktr May 22, 2020 •

edited

Loading

trusktr May 22, 2020 •

edited

Loading

trusktr May 22, 2020

trusktr May 22, 2020 •

edited

Loading

trusktr May 22, 2020 •

edited

Loading

trusktr May 22, 2020

anikethsaha commented May 22, 2020

trusktr commented May 23, 2020

anikethsaha commented May 23, 2020

trusktr commented Jun 17, 2020

WIP, updating server.js to use JSDOM to fix isExternal, but also some… #3

WIP, updating server.js to use JSDOM to fix isExternal, but also some… #3

Conversation

trusktr commented May 22, 2020

trusktr May 22, 2020

Choose a reason for hiding this comment

trusktr May 22, 2020

Choose a reason for hiding this comment

trusktr May 22, 2020

Choose a reason for hiding this comment

trusktr May 22, 2020

Choose a reason for hiding this comment

trusktr May 22, 2020

Choose a reason for hiding this comment

trusktr May 22, 2020 • edited Loading

Choose a reason for hiding this comment

trusktr May 22, 2020 • edited Loading

Choose a reason for hiding this comment

trusktr May 22, 2020 • edited Loading

Choose a reason for hiding this comment

trusktr May 22, 2020

Choose a reason for hiding this comment

trusktr May 22, 2020 • edited Loading

Choose a reason for hiding this comment

trusktr May 22, 2020 • edited Loading

Choose a reason for hiding this comment

trusktr May 22, 2020

Choose a reason for hiding this comment

anikethsaha commented May 22, 2020

trusktr commented May 23, 2020

anikethsaha commented May 23, 2020

trusktr commented Jun 17, 2020

trusktr May 22, 2020 •

edited

Loading

trusktr May 22, 2020 •

edited

Loading

trusktr May 22, 2020 •

edited

Loading

trusktr May 22, 2020 •

edited

Loading

trusktr May 22, 2020 •

edited

Loading