Enable server-side/isomorphic rendering #360

codebling · 2016-12-13T03:52:22Z

Fixes #131

Checklist

Tests run successfully. No test updates needed.
No documentation changes necessary.

Changes proposed in this pull request:

Resolves a few minor issues which prevent Blueprint from rendering on the server.

…default to null to fix server-side rendering.

… default to "ctrl" (as on all non-Apple platforms) to fix server-side rendering.

…es server-side rendering.

…ber before attempting to use it. Fixes server-side rendering.

…endering, since 'componentDidMount' never gets called on the server ('componentWillMount' does).

…ger more useable by not exploding when it called during the server lifecycle.

palantirtech · 2016-12-13T03:52:23Z

Thanks for your interest in palantir/blueprint, @codebling! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

codebling · 2016-12-13T04:12:18Z

I've enabled CircleCI, but I can't for the life of me get it to build the right branch.

codebling · 2016-12-13T04:36:20Z

Got CircleCI to work. Linting currently fails. I thought linting was performed in the test task. Fixing now.

adidahiya · 2016-12-13T05:07:15Z

packages/core/src/components/hotkeys/hotkeysTarget.tsx

+        }
+    };
+
+    // tslint:disable no-invalid-this only-arrow-functions


you don't need this line; L41 disables these rules for the rest of the file until a tslint:enable is encountered.

Ah, good catch. That was a quick copy/paste and I'm not super familiar with TSLint.

adidahiya · 2016-12-13T05:09:30Z

packages/core/src/components/hotkeys/hotkeysEvents.ts

@@ -13,7 +13,7 @@ import { comboMatches, getKeyCombo, IKeyCombo, parseKeyCombo } from "./hotkeyPar
 import { IHotkeysProps } from "./hotkeys";
 import { isHotkeysDialogShowing, showHotkeysDialog } from "./hotkeysDialog";

-const SHOW_DIALOG_KEY_COMBO = parseKeyCombo("?");
+const SHOW_DIALOG_KEY = "?";


adidahiya · 2016-12-13T05:13:01Z

packages/core/src/accessibility/focusStyleManager.ts

+};
+const focusEngine = typeof document !== "undefined"
+    ? new InteractionModeEngine(document.documentElement, FOCUS_DISABLED_CLASS)
+    : fakeFocusEngine;

 // this is basically meaningless to unit test; it requires manual UI testing
 /* istanbul ignore next */


Unit test coverage is failing for this file; it's pretty hard to cover this functionality anyway, so we should disable the whole file. I suggest moving this /* istanbul ignore next */ line to the top, right after the license.

Sounds good, thanks. I was just struggling with that. Coverage inexplicably runs on CircleCI but not locally, and I can't figure out why.

Have you tried ~~gulp karma-unit-core~~ gulp karma-core?

I've been using gulp test, which is an alias for gulp karma. gulp karma-core seems to do the same thing but only for the 'core' package. In all cases a coverage report is generated, but it is empty (shows 100% Statements 0/0 100% Branches 0/0 100% Functions 0/0 100% Lines 0/0). Anyhow, coverage seems to pass now.

…o trivial as to render extended testing useless.

adidahiya · 2016-12-13T05:21:59Z

packages/core/src/accessibility/focusStyleManager.ts

@@ -4,12 +4,20 @@
 * of the license at https://github.com/palantir/blueprint/blob/master/LICENSE
 * and https://github.com/palantir/blueprint/blob/master/PATENTS
 */
+/* istanbul ignore next */


Sorry, maybe it needs to be the first line of the file, before the license? I haven't tried this locally yet.

How does one try it locally? I can't find a task, and coverage doesn't run on gulp test for me the way it does on CircleCI

I'll look it up. ignore next seems to be used on an object-basis elsewhere in the package.

Try gulp karma-core

Seems to just ignore the next entire statement block. https://github.com/gotwarlost/istanbul/blob/master/ignoring-code-for-coverage.md

adidahiya · 2016-12-13T15:42:51Z

packages/core/src/components/index.ts

@@ -5,7 +5,12 @@
 * and https://github.com/palantir/blueprint/blob/master/PATENTS
 */

-import "dom4";
+declare function require(moduleName: string): any; // declare node.js "require" so that we can conditionally import


note for a follow-up change (doesn't have to be in this PR): we should share this ambient declaration with https://github.com/palantir/blueprint/blob/497a03eab4420e48db0865407995895aa25726fa/packages/docs/src/require-shim.d.ts

I included it there in the same file specifically because I was worried about scoping. It makes sense to merge them. Personally, I find the Node.js form clearer than the one used in require-shim, but whatever works!

adidahiya · 2016-12-13T15:53:00Z

I checked out this branch and did some regression testing. This looks good to merge. Thanks @codebling!

giladgray · 2016-12-13T16:52:18Z

packages/core/src/components/hotkeys/hotkeyParser.ts

@@ -112,7 +112,7 @@ export const Aliases = {
    command: "meta",
    escape: "esc",
    minus: "-",
-    mod: /Mac|iPod|iPhone|iPad/.test(navigator.platform) ? "meta" : "ctrl",
+    mod: ((typeof navigator !== "undefined") && /Mac|iPod|iPhone|iPad/.test(navigator.platform)) ? "meta" : "ctrl",


feels like we should have an isUndefined utility next to isFunction

giladgray · 2016-12-13T16:54:21Z

packages/core/src/components/menu/menuItem.tsx

-            right = document.documentElement.clientWidth - right;
+            if (typeof document !== "undefined"
+                && typeof document.documentElement !== "undefined"
+                && Number(document.documentElement.clientWidth)) {


implicit coercion number => boolean

Agreed. I spotted this, but unfortunately we can't test for NaN because NaN == NaN is never true. Number.isNaN() would be perfect, but only works if you're targeting es6. Otherwise, NaN correctly coerces to false, and the only other problematic value (coerces to a value we don't want) is 0, but if the clientWidth is 0 then it doesn't matter what the alignment is. So I left as-is. Only other option would be check if it's defined.

giladgray

wow that was easy! thank you @codebling 🎩 !

llorca · 2016-12-13T21:53:41Z

🔥 PR! Nice work @codebling

codebling · 2016-12-13T22:08:34Z

Looks like @giladgray has already updated the wiki. Perfect!

RobAWilkinson · 2016-12-16T21:12:49Z

Can we get a release for this?
Npm installs can't point to a git subdirectory where core is

adidahiya · 2016-12-16T21:14:48Z

@RobAWilkinson coming momentarily: #379

codebling added 7 commits December 12, 2016 19:13

Conditionally load DOM4 polyfill (load only in the browser)

e9c89e0

Check for presence of 'document' before attempting to use it. Safely …

f281672

…default to null to fix server-side rendering.

Check for presence of 'navigator' before attempting to use it. Safely…

32dda35

… default to "ctrl" (as on all non-Apple platforms) to fix server-side rendering.

Store the "show dialog" key rather than the key combo in a const. Fix…

345e1fa

…es server-side rendering.

Check that 'document.documentElement.clientWidth' exists and is a num…

69568c3

…ber before attempting to use it. Fixes server-side rendering.

Move event bindings to 'componentDidMount', which fixes server-side r…

f77bd1a

…endering, since 'componentDidMount' never gets called on the server ('componentWillMount' does).

Return an void/noop Manager in a server context. Makes FocusStyleMana…

dd490e9

…ger more useable by not exploding when it called during the server lifecycle.

This was referenced Dec 13, 2016

Remove dom4 as a dependency in core #358

Closed

SSR with blueprint #131

Closed

Fix lint

828acbe

adidahiya reviewed Dec 13, 2016

View reviewed changes

Don't calculate code-coverage for this file, since functionality is s…

df12c62

…o trivial as to render extended testing useless.

adidahiya reviewed Dec 13, 2016

View reviewed changes

codebling added 2 commits December 13, 2016 00:23

Remove useless tslint hint

6a53b13

Ignore coverage checks on code that can't be meaningfully tested.

97c0800

adidahiya approved these changes Dec 13, 2016

View reviewed changes

adidahiya reviewed Dec 13, 2016

View reviewed changes

adidahiya merged commit 7596b0a into palantir:master Dec 13, 2016

giladgray reviewed Dec 13, 2016

View reviewed changes

adidahiya mentioned this pull request Dec 13, 2016

Error: Debug Failure. False expression: <empty> #348

Closed

justingreenberg mentioned this pull request Dec 14, 2016

Add tests for SSR to prevent regressions #370

Closed

llorca mentioned this pull request Dec 16, 2016

Prepare release v1.4.0 #379

Merged

codebling mentioned this pull request Dec 18, 2016

Add test for server-side/isomorphic rendering #381

Merged

codebling mentioned this pull request Jul 6, 2020

Doc - Fix collection.exists indentation arangodb/arangojs#672

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable server-side/isomorphic rendering #360

Enable server-side/isomorphic rendering #360

codebling commented Dec 13, 2016

palantirtech commented Dec 13, 2016

codebling commented Dec 13, 2016

codebling commented Dec 13, 2016

adidahiya Dec 13, 2016

codebling Dec 13, 2016

adidahiya Dec 13, 2016

adidahiya Dec 13, 2016

codebling Dec 13, 2016

adidahiya Dec 13, 2016 •

edited

Loading

codebling Dec 13, 2016

adidahiya Dec 13, 2016

codebling Dec 13, 2016

codebling Dec 13, 2016

adidahiya Dec 13, 2016

codebling Dec 13, 2016

adidahiya Dec 13, 2016

codebling Dec 13, 2016 •

edited

Loading

adidahiya commented Dec 13, 2016 •

edited

Loading

giladgray Dec 13, 2016

giladgray Dec 13, 2016

codebling Dec 13, 2016

giladgray left a comment

llorca commented Dec 13, 2016

codebling commented Dec 13, 2016

RobAWilkinson commented Dec 16, 2016

adidahiya commented Dec 16, 2016

Enable server-side/isomorphic rendering #360

Enable server-side/isomorphic rendering #360

Conversation

codebling commented Dec 13, 2016

Fixes #131

Checklist

Changes proposed in this pull request:

palantirtech commented Dec 13, 2016

codebling commented Dec 13, 2016

codebling commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebling Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

adidahiya commented Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray left a comment

Choose a reason for hiding this comment

llorca commented Dec 13, 2016

codebling commented Dec 13, 2016

RobAWilkinson commented Dec 16, 2016

adidahiya commented Dec 16, 2016

adidahiya Dec 13, 2016 •

edited

Loading

codebling Dec 13, 2016 •

edited

Loading

adidahiya commented Dec 13, 2016 •

edited

Loading