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

Weird: wrong images get loaded #272

Closed
elbotho opened this issue May 29, 2020 · 11 comments
Closed

Weird: wrong images get loaded #272

elbotho opened this issue May 29, 2020 · 11 comments
Labels
bug Something isn't working
Milestone

Comments

@elbotho
Copy link
Member

elbotho commented May 29, 2020

In this table the colors are different from the same table in the old design:
https://de.serlo.org/community/hilfe-bearbeitung/richtlinien-inhalten/bilder-grafiken#SerloFarben

@elbotho elbotho added the bug Something isn't working label May 29, 2020
@inyono
Copy link
Member

inyono commented May 29, 2020

Seems like SSR is okay, but something is wrong on the client side (or caching, who knows): If I make a hard reload, I notice that the correct colors are shown shortly and then change to the wrong colors.

@inyono
Copy link
Member

inyono commented May 29, 2020

Yep, deactivating JS has the same effect.

@Entkenntnis
Copy link
Member

Interestingly, this does not happen when you open the page directly in the frontend.

correct: https://frontend.serlo.now.sh/community/hilfe-bearbeitung/richtlinien-inhalten/bilder-grafiken#SerloFarben

grafik

wrong (frontend enabled): https://de.serlo.org/community/hilfe-bearbeitung/richtlinien-inhalten/bilder-grafiken#SerloFarben

grafik

That's quite weird ...

@inyono
Copy link
Member

inyono commented Jun 5, 2020

You need to check https://frontend.serlo.org/community/hilfe-bearbeitung/richtlinien-inhalten/bilder-grafiken#SerloFarben since that is the production domain (frontend.serlo.now.sh uses staging). But still the same behavior ^^. Probably should to check the table renderer if we do anything that might trip React up (key handling, I don't know)

@Entkenntnis Entkenntnis added this to the 100% FRONTEND milestone Jun 8, 2020
@elbotho
Copy link
Member Author

elbotho commented Jun 8, 2020

I think it's curious that it's the only render function in article-renderer.tsx that explicitly sets the key:

export function renderTable({ attributes = {}, children = null }: any) {
  const { key, ...otherAttribs } = attributes
  return (
    <TableWrapper key={key}>
      <StyledTable>
        <tbody {...otherAttribs}>{children}</tbody>
      </StyledTable>
    </TableWrapper>
  )
}

@Entkenntnis do you think this would make sense to test?
I don't see what attributes would need to go to tbody anyway.
(currently attributes only contains the key)

export function renderTable({ attributes = {}, children = null }: any) {
  // const { key, ...otherAttribs } = attributes
  return (
    <TableWrapper {...attributes}>
      <StyledTable>
        <tbody>{children}</tbody>
      </StyledTable>
    </TableWrapper>
  )
}

@elbotho
Copy link
Member Author

elbotho commented Jun 8, 2020

PR here: #272

@inyono inyono closed this as completed in c9762ab Jun 8, 2020
@elbotho elbotho reopened this Jun 8, 2020
@elbotho
Copy link
Member Author

elbotho commented Jun 8, 2020

Okay, so strange. I'm out out of ideas.

@inyono
Copy link
Member

inyono commented Jun 8, 2020

Mh... for one thing: we don't let React handle stuff but rather do it ourselves (e.g. we create JSX nodes ourselves instead of rendering react components; also passing down keys ourselves). This looks like an anti-pattern to me...

@Entkenntnis
Copy link
Member

Entkenntnis commented Jun 8, 2020

I found a possible cause: Diffing the HTML obtained from frontend.serlo.org and de.serlo.org reveals that Cloudflare cleans up the HTML and especially removes all comments.

These comments are there to help react to hydrate the page (see facebook/react#14725 (comment)), because the comments are missing, the hydrating leads to dom mutations (which it shouldn't!) and a corrupt dom.

On the left is the cloudflare result, you can see the comments missing from the table:

grafik

@inyono Do we have "minify html" activated? This could cause this issue.

@inyono
Copy link
Member

inyono commented Jun 8, 2020

Nice catch! This seems to be the problem, indeed. I deactivated the Minifier from Cloudflare and purged the cache for that page. Seems to work now.

@inyono inyono closed this as completed Jun 8, 2020
@elbotho
Copy link
Member Author

elbotho commented Jun 8, 2020

nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants