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

@vx/text width prop breaks w/ SSR #266

Open
brandonmp opened this issue Apr 8, 2018 · 12 comments
Open

@vx/text width prop breaks w/ SSR #266

brandonmp opened this issue Apr 8, 2018 · 12 comments
Labels

Comments

@brandonmp
Copy link

(I'm not actually sure if this has anything to do with SSR, but the problem presents only on fresh page loads in my nextjs project, so that's normally the culprit.)

I'm calling the component like so:

 <Text
    scaleToFit
    width={25}
    x={9}
    textAnchor={'middle'}
    y={18}
    angle={-90}
 >
Some label
</Text>

On page load, however, the rendered styles on the <text> element are broken. specifically, the transform value is: matrix(Infinity, 0, 0, Infinity, -Infinity, -Infinity) rotate(-90, 9, 18)

The practical implication of this bad transform in my case is that text I'm trying to render vertically shows up w/ a horizontal orientation.

e.g. instead of this:
image

it renders this:
image

When hot reloading updates the bundle, however, the transform prop gets updated to a valid value, and the text renders as it ought to:

matrix(0.7368114063658365, 0, 0, 0.7368114063658365, 2.3686973427074713, 4.737394685414943) rotate(-90, 9, 18)

I thought maybe it was the memoization (i think?) pattern you've got going on in componentWillReceiveProps, so I tried shorting that by creating / passing a new style object on render, but no such luck.

@techniq
Copy link
Collaborator

techniq commented Apr 9, 2018 via email

@hshoff
Copy link
Member

hshoff commented Apr 9, 2018

hmm that's odd. I'm not seeing any errors when loading https://vx-demo.now.sh/text which uses nextjs...even tried making <Text /> with all the props in your example above.

To help me reproduce this problem, which version of next are you using?

@brandonmp
Copy link
Author

brandonmp commented Apr 9, 2018

@hshoff I'm using next@5.1.0. I just threw in regular <text> elems for now & they're working fine, so I'm assuming it's something in the calcs in @vx/text

The docs look like they're referencing an old version maybe? the component in the docs is <TextOutline>, which i assume is diff't from the <Text> component. Either way, it doesn't look like the example is using scaleToFit, and since that's the prop that results in the matrix transform being added, I figure that's why the bad transform isn't presenting in docs.

ETA just re-read your note that you tried w/ same props from example, so scratch this theory. looking at src I'm thinking it has to be getStringWidth, but not sure why docs wouldn't present

@techniq I think you're right. It looks like getStringWidth is called in Text.componentWillMount, which fires on the server.

Normally when document gets referenced on the server, it throws, but it looks like getStringWidth is wrapped in a try/catch & just silently returning null. I think it's those null values that are resulting in the Infinitys making their way into the matrix transform, thus breaking shit

@brandonmp
Copy link
Author

i was able to reproduce it in the docs by modifying text.js to this:

import React, { Component } from 'react';
import { Text } from '@vx/text';

export default () => (
    <div
        style={{
            display: 'flex',
            width: '90vw',
            height: '90vh',
            justifyContent: 'center',
            alignItems: 'center'
        }}
    >
        <svg width={500} height={200}>
            <Text
                scaleToFit
                width={25}
                x={250}
                textAnchor={'middle'}
                y={18}
                angle={-90}
            >
                This should be vertical
            </Text>
        </svg>
        <svg width={100} height={200}>
            <Text width={25} x={15} textAnchor={'middle'} y={25} angle={-90}>
                This is vertical
            </Text>
        </svg>
    </div>
);

here's a live demo

@techniq
Copy link
Collaborator

techniq commented Apr 9, 2018

Recharts (where I originally submitted this component too) has a SSR check before calculating the word widths that we should do as well. I haven't used SSR much myself so I've never ran into this. I'm guessing if we don't run this on the server, it should run the calculations during the client render.

@hshoff hshoff added the 🐛 bug label Apr 9, 2018
@williaster
Copy link
Collaborator

I'm also less familiar with SSR, can this be fixed with the SSR check + moving it to componentDidMount to compute upon client render?

@techniq
Copy link
Collaborator

techniq commented Apr 9, 2018

@williaster we need to recalculate the word widths any time the children (i.e. text) or styles change so it needs to happen anytime the props change (although this will need to be changed to getDerivedStateFromProps in the future).

@brandonmp
Copy link
Author

brandonmp commented Apr 16, 2018

@techniq is it good enough to just put a ballpark value based on string length for SSR, then calc the real value when the component mounts?

it's admittedly a 'good enough' solution, but i've used similar approaches in a few cases working around SSR constraints.

so, e.g., where getStringWidth.js returns null on the server:

...
function getStringWidth(str, style) {
  try {
  } catch (e) {
    return null;
  }

you could simply do

...
function getStringWidth(str, style) {
  try {
  } catch (e) {
    return (str || '').width * 3; // 3 being an arbitrary 'pixel width per char' number
  }

Could also add a pixelsPerCharacter prop to the component to let lib users adjust to their needs. eg:

...
function getStringWidth(str, style, pxPerChar = 3) {
  try {
  } catch (e) {
    return (str || '').width * pxPerChar; 
  }

@brandonmp
Copy link
Author

brandonmp commented Apr 16, 2018

also: the typical way i see ssr checked is typeof window === 'undefined', like in the snip from recharts

i like the try/catch approach, but maybe a more explicit check is better here

@techniq
Copy link
Collaborator

techniq commented Apr 16, 2018

@brandonmp seems reasonable. I'm not very familiar with SSR and how state is reconstituted on the client. Would want to make sure another calculation occurred on the client to correct the values.

Also, seems like matching how recharts handles this would work. Changing this to

if ((props.width || props.scaleToFit) && typeof window !== 'undefined') {

@williaster
Copy link
Collaborator

was there any alignment on the desired approach here? simply adding the line @techniq called out?

if ((props.width || props.scaleToFit) && typeof window !== 'undefined') {

@techniq
Copy link
Collaborator

techniq commented Jun 26, 2018 via email

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

No branches or pull requests

4 participants