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

SSR with blueprint #131

Closed
RobAWilkinson opened this issue Nov 13, 2016 · 27 comments
Closed

SSR with blueprint #131

RobAWilkinson opened this issue Nov 13, 2016 · 27 comments

Comments

@RobAWilkinson
Copy link

Server side rendering with blueprint breaks, dom4.max.js is requiring tons of things.

@giladgray
Copy link
Contributor

giladgray commented Nov 13, 2016

interesting! we've never used blueprint in an SSR app so this has never come up before. as a result, SSR support is low priority for the team because we don't build apps like this.

@RobAWilkinson are you having trouble getting dom4 to work on the server? can you be more specific about the problem?


11/17/16 update from @adidahiya, so as not to have the response buried:

We're going to look at server side rendering more seriously some time in the next couple months. Watch this issue for updates.

@RobAWilkinson
Copy link
Author

Ahh gotcha.

Yeah it seems that dom4 is pretty client specific, I went down the path of mocking window and document with jsdom but pretty soon that became untenable.

dom4 is looking for things like DocumentFragment and extending their prototype chain, grabbing window.Node etc. This is all scattered throughout the 1k library.

Are there specific components that are using dom4 more than others? I would assume things like datetime are doing js DOM manipulations heavily which is why its moved out of core.
I'm wondering if we could extend that pattern to break up blueprint even more and have some of the more simple things in a package that is SSR usable.

Ex:
I would imagine that things like the InputGroup Button are mostly stateless functional components (or have a small amount of state) and do not manipulate the dom at all.

Currently I'm doing this myself building on BlueButton components, BlueInput components and using the css classes inside those.

@schrepfler
Copy link

schrepfler commented Nov 14, 2016

Hi, I was experimenting with next.js and tried to add your blueprints but I got this exception as well. Do you think you can split somehow the component so both client side and server side rendering is possible?
I think for some SSR is a must.

@isTravis
Copy link

+1 to @schrepfler's point that SSR is a must for some.

My quick and ugly hack is the following:

  1. For many things (e.g. tags, nav) the CSS API is sufficient. It'd be nice to use the JS API - but oh well.
  2. For those that require the JS API because of interactivity (e.g. Popover, Alerts), I create DefaultComponent as follows
function DefaultComponent(props) {
  return <div>{this.props.children}</div>;
}

I then do some shady componentDidMount stuff like so:

let Popover = defaultComponent; // Initialize Popover to be the default component
export const MyComponent = React.createClass({
    componentDidMount() {
        Popover = require('@blueprintjs/core').Popover; // Overwrite the default component with the real component
        this.forceUpdate(); // Trigger a re-render
    },
    render() {
        return (
            <div>
                <Popover 
                    content={<div>This is my popover content</div>}
                    popoverClassName="pt-popover-content-sizing"
                    useSmartPositioning={true}
                >
                    <button className="pt-button">Click for Popover</button>
                </Popover>
            </div>
        )
    }
});

Because DefaultComponent renders {this.props.children}, you won't notice any flicker on the button on first load, and once the JS loads client-side everything will function as expected.

It's ugly, it makes my insides hurt - but for many of the components I'm excited about, this'll work for the time being. It won't work for things that need the JS API and have inherit style as they will flicker on load.

I also tried messing with JSdom in dom4, and faking window, document, etc in the blueprint source - but as @RobAWilkinson said, it gets pretty untenable.

Thanks for all this work - excited to see it progress!

@RobAWilkinson
Copy link
Author

Thanks for this great tip, this is awesome but just warning everyone, if you try to do this with some complicated components, that internally use refs i.e. Toaster you quickly start getting really weird errors about double refs etc

@abhishiv
Copy link

abhishiv commented Nov 19, 2016

For anyone else who's stuck at this - I have been able to to fix this by manually patching following file:

cp ./patches/blueprint/components.js ./node_modules/@blueprintjs/core/dist/components/index.js
cp ./patches/blueprint/index.js ./node_modules/@blueprintjs/core/dist/index.js
cp ./patches/blueprint/hotkeyParser.js  ./node_modules/@blueprintjs/core/dist/components/hotkeys/hotkeyParser.js

I have npm run patch, which automatically swaps these files in my build pipeline. Also I include dom4.js via a script tag in page, since I commented it out in the patched file.

Seems to work fine so far, although haven't tested all the components. If anyone finds any memory leaks during SSR, please let me know.

@schrepfler
Copy link

Is that available via some PR @abhishiv?

@abhishiv
Copy link

@schrepfler no, but I put them up in a gist in case anyone needs it. Simple stuff like checking for navigator existence etc.

https://gist.github.com/anonymous/7d011dddb77fbce1f378f62f77553c58

@schrepfler
Copy link

What does this patch do? Why is dom4j strictly needed on the server side, can't the react dom be used instead?

@RobAWilkinson
Copy link
Author

I believe it is mainly being used for it's keyboard events.

@abhishiv
Copy link

abhishiv commented Nov 20, 2016

Yeah, and for some other stuff I think. These patches, comment the line including dom4j, so it doesn't get loaded on the server. However on the client, you need to include it so that things work - I did it via a script tag.

The other two files are patched to check for existence of document/navigator.

@DClark5218
Copy link

@abhishiv are you saying this was patched and should work now?

@abhishiv
Copy link

@DClark5218 Only if you patch it yourself using the files I uploaded.

@justingreenberg
Copy link

justingreenberg commented Nov 21, 2016

you can see a diff here of some of my SSR research over the weekend applied to core:
master...justingreenberg:jg/server-rendering

summary...

  1. utils/isBrowser - simple guard for detecting execution context

image

  1. dom4 - load conditionally based on environment:

image

  1. InteractionModeEngine - set up a sink for browser events in node...

image

  1. hotkeyParser - super simple fix

image

these fixes will effectively render the markup, but anything relying on popover/portal will trigger a re-render on client (checksum mismatch) and will break event handlers due to direct dom manipulation related to Tether integration:

// portal.tsx
// ...
const targetElement = document.createElement("div");
targetElement.classList.add(Classes.PORTAL);
document.body.appendChild(targetElement);
this.targetElement = targetElement;

bypassing this on server is easy and will stop checksum warnings since dom is the same, but now we've lost an HTMLElement that react expects as target so throws reference error

i didn't have time to look into a full migration this weekend, but based on my research these issues could likely be mitigated by replacing ad hoc portal solution with react-portal or react-tether, both of which seem to have addressed these issues using controlled component state


@abhishiv have you tested your patch with overlays and tooltips?

class PasswordField extends Component {
  toggleVisbile = e => {
    e.preventDefault()
    e.stopPropagation()
    this.props.toggle()
  }
  
  render () {
    const { showPassword, setText } = this.props

    let button = (
      <Tooltip content={`${showPassword ? "Hide" : "Show"} Password`}>
        <Button
          className={Classes.MINIMAL}
          intent={Intent.WARNING}
          iconName={showPassword ? "unlock" : "lock"}
          onClick={this.toggleVisbile}
        />
      </Tooltip>
    )

    return (
      <div className="pt-app">
        <InputGroup
          className="pt-large"
          placeholder="Enter your password..."
          rightElement={button}
          type={showPassword ? "text" : "password"}
          onChange={e => setText(e.target.value.trim())}
        />
      </div>
    )
  }
}

AFAICT this will break, since once in dom context react will not be able to find Tooltip portal's target element

@abhishiv
Copy link

abhishiv commented Nov 21, 2016

Hey @justingreenberg, weirdly for me it works both on client/server without any warnings/errors. It's weird, because our patches are almost the same.

The function you wrote about in portal.tsx is run in componentDidMount, so I don't think it should break anything.

How does it break for you? Can you share the warnings/errors you get?

@schrepfler
Copy link

Can we get a build with this patch?

@llorca
Copy link
Contributor

llorca commented Nov 29, 2016

@schrepfler sorry, we won't just release a quick patch for this. It's something we care about and very much want to do right -- we're investigating SSR now and once we're solid on all the use cases, we'll do a release with first-class SSR support. In the meantime, definitely feel free to experiment with the changes proposed above.

@adidahiya anything you'd like to add?

@codebling
Copy link
Contributor

codebling commented Dec 13, 2016

I started compiling a list of fixes needed before I found this post. I eventually found the number of fixes needed to be very minimal, and so made them.
@justingreenberg has done some good research, and I've checked my work with his. Here's what he's done differently:

  • Addition of isBrowser method - I think this is the wrong approach. We should be checking for the presence for the individual values we need. Adding an isBrowser method encourages people to use it.
  • Conditional load of dom4 - this is the right approach, but this actually breaks the build if the types are not added to the package.
  • InteractionModeEngine - simply passing null to the InteractionModeEngine (instead of document) is safe as long as it is only used in browser-only lifecycle. Passing a void functions does make it easier on the average user, however, so that is probably the better solution.
  • HotkeyParser - looks good to me, though my preference is to check directly for the presence of navigator.
  • A few more fixes needed. See below.

Some points need to be addressed to ensure Blueprintjs' server rendering is maintained going forward.

  1. We should update the coding guidelines to ensure that browser APIs are not called during the server lifecycle.

    • constructor(), componentWillMount() and render() are called during server-side rendering and so cannot use browser APIs.
    • componentDidMount(), componentWillReceiveProps(), shouldComponentUpdate(), componentWillUpdate(), componentDidUpdate() are not called not called during server-side rendering and so can safely use browser APIs.
    • In particular, the section which reads "Bind component class methods upon declaration, not in the render() method" needs to be updated. Event bindings and code which mutates the DOM should be in componentDidMount().
  2. We should add a test to ensure that all components can be rendered on the server.

You can find my fork with the changes here. [EDIT: fork has been deleted, since the PR was merged. See #360 and commit 7596b0]
This compiles correctly and all tests are passing. See #360. Let me know if anything else needs to be patched.
Here's the list of all references to browser-specific variables in the code. BAD means that the code causes problems for server-side rendering and needs to be fixed. These are fixed in my fork (except for the Web Audio example). OK means that the code will never run when rendering on the server. I've ignored the docs, packages/docs, packages/landing directories, since these do not contain any code that end-users will ever need to server-render. The list is sorted by relative file name. References on different lines of the same file are considered together only when they are in the same method.

  • References to document
    • BAD - attempts to pass the root element to a constructor (such that it can attach event listeners). To fix, default to null for server-side rendering or, for useability, replace with a Manager that does nothing
      • packages/core/src/accessibility/focusStyleManager.ts:12
    • OK - event handler, will never be called on server
      • packages/core/src/components/context-menu/contextMenu.tsx:81
    • OK - only called statically outside of a React context, will never be called on server
      • packages/core/src/components/context-menu/contextMenu.tsx:102
      • packages/core/src/components/context-menu/contextMenu.tsx:104
    • OK - will never be called on the server
      • packages/core/src/components/editable-text/editableText.tsx:309
    • OK - will never be called on the server
      • packages/core/src/components/hotkeys/hotkeysDialog.tsx:82
      • packages/core/src/components/hotkeys/hotkeysDialog.tsx:84
    • BAD - move to componentDidMount
      • packages/core/src/components/hotkeys/hotkeysTarget.tsx:41
      • packages/core/src/components/hotkeys/hotkeysTarget.tsx:42
    • OK - componentWillUnmount will never be called on the server
      • packages/core/src/components/hotkeys/hotkeysTarget.tsx:51
      • packages/core/src/components/hotkeys/hotkeysTarget.tsx:52
    • BAD - calculates widths to decide whether to align left or right when using smart positioning. Fix by leaving default values if document.documentElement is not available. Smartpositioning will fix the alignment in browser, if needed.
      • packages/core/src/components/menu/menuItem.tsx:179
    • OK - won't be called server-side
      • packages/core/src/components/overlay/overlay.tsx:249
      • packages/core/src/components/overlay/overlay.tsx:250
      • packages/core/src/components/overlay/overlay.tsx:252
      • packages/core/src/components/overlay/overlay.tsx:260
    • OK - won't be called server-side
      • packages/core/src/components/overlay/overlay.tsx:268
      • packages/core/src/components/overlay/overlay.tsx:273
      • packages/core/src/components/overlay/overlay.tsx:276
      • packages/core/src/components/overlay/overlay.tsx:285
    • OK - won't be called server-side
      • packages/core/src/components/overlay/overlay.tsx:294
      • packages/core/src/components/overlay/overlay.tsx:298
    • OK - won't be called server-side
      • packages/core/src/components/portal/portal.tsx:43
      • packages/core/src/components/portal/portal.tsx:45
    • OK - won't be called server-side
      • packages/core/src/components/slider/handle.tsx:92
      • packages/core/src/components/slider/handle.tsx:93
    • OK - won't be called server-side
      • packages/core/src/components/slider/handle.tsx:99
      • packages/core/src/components/slider/handle.tsx:100
      • packages/core/src/components/slider/handle.tsx:101
    • OK - won't be called server-side
      • packages/core/src/components/slider/handle.tsx:178
      • packages/core/src/components/slider/handle.tsx:179
      • packages/core/src/components/slider/handle.tsx:180
      • packages/core/src/components/slider/handle.tsx:181
      • packages/core/src/components/slider/handle.tsx:182
    • OK - won't be called server-side
      • packages/core/src/components/tabs/tabs.tsx:283
    • OK - meant to be called statically outside of a React context, will never be called on server
      • packages/core/src/components/toast/toaster.tsx:91
      • packages/core/src/components/toast/toaster.tsx:92
    • OK - won't be called server-side
      • packages/datetime/src/common/utils.ts:12
      • packages/datetime/src/common/utils.ts:13
    • OK - won't be called server-side
      • packages/table/src/common/clipboard.ts:35
      • packages/table/src/common/clipboard.ts:38
      • packages/table/src/common/clipboard.ts:40
    • OK - won't be called server-side
      • packages/table/src/common/clipboard.ts:56
    • OK - won't be called server-side
      • packages/table/src/common/clipboard.ts:87
      • packages/table/src/common/clipboard.ts:104
      • packages/table/src/common/clipboard.ts:108
    • OK - won't be called server-side
      • packages/table/src/common/clipboard.ts:117
    • OK - won't be called server-side
      • packages/table/src/common/utils.ts:179
    • OK - won't be called server-side
      • packages/table/src/interactions/dragEvents.ts:53
      • packages/table/src/interactions/dragEvents.ts:54
    • OK - won't be called server-side
      • packages/table/src/interactions/dragEvents.ts:58
      • packages/table/src/interactions/dragEvents.ts:59
    • OK - won't be called server-side
      • packages/table/src/interactions/resizeSensor.ts:22
  • References to window
    • BAD - must be refactored to lazy-init all Web Audio API calls. This is only an example, though
      • packages/core/examples/hotkeyPiano.tsx:147
    • OK - won't be called server-side
      • packages/table/src/common/clipboard.ts:89
      • packages/table/src/common/clipboard.ts:96
  • References to navigator
    • BAD - used to provide a custom override for Apple systems, in multiple places. Can default to 'ctrl' on server, this is the default on all non-Apple platforms including gaming systems. This may cause incogruous behaviour on server vs browser, but I don't have a better solution at the moment.
      • packages/core/src/components/hotkeys/hotkeyParser.ts:115
  • Other issues
    • BAD - imports browser-only module 'dom4' to polyfill DOM level 4 features. This module needs to be conditionally loaded based on whether the environment is the server or the browser. This can be acheived as either a conditional import or both a webpack and a browserify configuration. The advantage of the conditional import is that it's more clear and visible. The disadvantage of the conditional import is that it requires another module (SystemJS), because import statements can only be at the top level in ES6/TypeScript. The advantage of a combined webpack/browserify configuration is that no additional module is needed
      • packages/core/src/components/index.ts:8

@codebling
Copy link
Contributor

Sorry for that megapost. A few more comments:

@justingreenberg the popover/portal code is fine. As mentioned by @abhishiv, this is called from componentDidMount, which is never called during server rendering.
@justingreenberg we should see to it that calls such as the one to document.createElement do not run on the server. In doing so we can avoid having to shim document on the server. The calls to createElement also don't create elements in the React-managed part of the DOM. With this in mind, there should be nothing to cause a checksum mismatch and re-render on the client.

@RobAWilkinson @schrepfler DOM4 is a polyfill for the W3C DOM level 4 living specification. Because this is related to DOM manipulation, it isn't needed server-side (but is still be needed browser-side).

@isTravis that's an interesting approach, but seems overcomplicated given that the amount of patching needed to get Blueprintjs to work server-side is minimal.

@justingreenberg your patch for conditional import of 'dom4' breaks the build

packages\core\src\components\overlay\overlay.tsx(301,55): error TS2339: Property 'query' does not exist on type 'HTMLElement'.

unless the dom4 types (required for TypeScript to correctly compile) are included. This is an easy change, though:

===================================================================
--- packages/core/package.json	(revision 497a03eab4420e48db0865407995895aa25726fa)
+++ packages/core/package.json	(revision )
@@ -6,6 +6,7 @@
   "typings": "dist/index.d.ts",
   "style": "dist/blueprint.css",
   "dependencies": {
+    "@types/dom4": "^1.5.20",
     "classnames": "^2.2",
     "dom4": "^1.8",
     "normalize.css": "4.1.1",

@RobAWilkinson
Copy link
Author

Awesome great work @codebling thanks for spending the time on this, I'll be using your fork until this gets merged.

@Timmmm
Copy link

Timmmm commented Feb 23, 2022

Just in case anyone comes across this while trying to use Blueprint SSR in Deno (or ESBuild, which Deno uses). Unfortunately it doesn't seem to work. You get these errors:

error: Uncaught ReferenceError: Element is not defined
    at https://cdn.esm.sh/v66/dom4@2.1.6/deno/dom4.js:2:1922
    at https://cdn.esm.sh/v66/dom4@2.1.6/deno/dom4.js:2:9117
    at https://cdn.esm.sh/v66/dom4@2.1.6/deno/dom4.js:2:250
    at https://cdn.esm.sh/v66/dom4@2.1.6/deno/dom4.js:2:11537

I suspect this is because ESBuild does not support conditional imports like the one Blueprint uses; basically:

if (thisIsABrowser) {
   require("dom4");
}

And that is conflated with the fact that dom4 has side effects when it is imported (terrible design). The clean solutions is to change dom4 so it doesn't have side effects on import, but requires a registerDom4(window); function call. Then you can just do

import { registerDom4 } from "dom4";

if (thisIsABrowser) {
   registerDom4(window);
}

But seeing as that requires modifying dom4 in a backwards incompatible way I won't hold my breath.

I don't have a solution, sorry. Just posting this here because it's high in the Google results for relevant searches.

@adidahiya
Copy link
Contributor

ESBuild does not support conditional imports like the one Blueprint uses

sounds like a good feature request for ESBuild?

@codebling
Copy link
Contributor

Thanks for the info @Timmmm !

I think there are a few potential solutions here (though I'm not 100% sure about the feasibility of any of them),

  1. are you able to use one of the distribution files for Blueprint that has already been built with Webpack?
  2. dom4 could be wrapped in a new lib such that it has no side-effects, and used as you described
  3. we could perhaps use import() instead of require() here

What do you think about these? If you come up with a solution and open a PR, my experience has been that the team here has been very receptive :)

@Timmmm
Copy link

Timmmm commented Feb 24, 2022

Yeah it does feel like something esbuild could support. Thanks for the ideas:

  1. I don't think easily - I'm already hackily using it through https://esm.sh/
  2. Yeah that would definitely be a good idea, though I looked into the code and it seems like there are possibly other side effect browser-only modules, e.g. whatwg-fetch? So it may be just the first bit of a big job.
  3. I have no idea if that would work - I would have thought esbuild treats them the same?

Anyway I gave up on Deno for now and went back to Node. I suspect the most prudent action is simply to wait until you guys drop IE support and then all the polyfills can just be removed (as far as I can tell only IE needs the dom4 or fetch polyfills).

Actually now that I think about it I'm still using esbuild but directly instead of through esm.sh and it works fine. Weird. This legacy module stuff is a mess!

@adidahiya
Copy link
Contributor

wait until you guys drop IE support and then all the polyfills can just be removed (as far as I can tell only IE needs the dom4 or fetch polyfills)

yes, this is one option. note that this is coming later this year with v5.0: #5143

@codebling
Copy link
Contributor

For 2, I'd meant to include more details. I was thinking it might be possible to create temporary globals right before dom4 is required/imported, then restore them afterwards and hook the side-effect function. Probably more work than it's worth.

Actually Webpack can target specific browsers and do any transpilation, courtesy Babel, so this library probably isn't required at all.

  1. import() is a Javascript feature, so I think esbuild has to support it.

Looks like you're back to Node for now. Thanks for sharing your experience, I have not made the jump to Deno yet, for a few reasons, but I am keeping a close eye on it.

yes, this is one option. note that this is coming later this year with v5.0: #5143

nice!

@Timmmm
Copy link

Timmmm commented Feb 24, 2022

import() is a Javascript feature, so I think esbuild has to support it.

I agree.

yes, this is one option. note that this is coming later this year with v5.0: #5143

Ah nice. That is definitely the sensible solution!

I have not made the jump to Deno yet, for a few reasons, but I am keeping a close eye on it.

I only have limited experience but I'm sold. It's definitely the future, if not quite the present.

The URL import system feels very weird at first but after using it for a while I think it's actually the best way to go.

It's especially good for replacing shell scripts. You get to write single file scripts in a sane statically typed language and you can use third party libraries without some extra project config file. I don't think anything else can offer that.

Anyway I shall look forward to Blueprint 5. Great project btw!

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

No branches or pull requests