bloomreach / spa-sdk

Apache License 2.0
15 stars 16 forks source link

async rewriteLinks function breaks example #8

Closed sergiomensing closed 1 year ago

sergiomensing commented 1 year ago

Hi again, We are trying to upgrade the SDK in our frontend project (next) to 18.0.1

While updating we encountered an issue with the breaking change of making the rewriteLinks function asynchronous

That makes this example invalid: https://github.com/bloomreach/spa-sdk/blob/main/packages/spa-sdk/README.md#rendering-html-content-safely

The example:

{/* Suppose the content.value below contains HTML markups string. */}
<div>
   {content && <div dangerouslySetInnerHTML={{ __html: page.rewriteLinks(page.sanitize(content.value)) }} />}
</div>

I see in the release notes an ADDITIONAL NOTE about async rendering. Is that the same issue or is this an unrelated problem?

joerideg commented 1 year ago

hi @sergiomensing, Thanks for reporting this. We will take a look at and update the description of the sanitize example.

The example app code does already show the correct way of sanitizing the HTML (and rewriting the links), this example and others like it in the README will have to be updated.

sergiomensing commented 1 year ago

Hey @joerideg, thanks for your response.

I think you're referring to this example where the useHTML hook is used.

We are using SSR in our Next.js project. This makes the useHTML hook not usable for us. Because useHTML uses the useEffect hook under the hood, the sanitised html and rewritten links are only rendered on the client when the page gets hydrated.

I know the release notes of 18.0.1 say:

It was brought to our attention that when using React SSR, but not Next.js, async rendering is not possible.

But I don't see how SSR with Next.js is possible in this situation. Do you maybe have an example of that?

drq commented 1 year ago

The useHTML seems to only work for 'content' and it doesn't seem to work for other fields such as 'header', 'description' etc.

We instead use following code

` const [header, setHeader] = useState(''); const [footer, setFooter] = useState('');

......

page?.rewriteLinks(content.header ?? '').then(text => { setHeader(text); });

page?.rewriteLinks(content.footer ?? '').then(text => { setFooter(text); });

`

sergiomensing commented 1 year ago

Hi @drq, thanks for your example!

I assume the rewriteLink promises are wrapped inside a useEffect hook? If that's the case, the content still does not render server-side right, or am I missing something?

sergiomensing commented 1 year ago

@joerideg, do you maybe have an update for us on this issue?

joerideg commented 1 year ago

hi @sergiomensing no not really unfortunately. I realize now that useEffect never runs on SSR... but also getServerSideProps obviously runs only for pages not for components. Will have to investigate how one could run an async function on component level when doing SSR.

joerideg commented 1 year ago

Created https://issues.onehippo.com/browse/SPASDK-164 for progress tracking

sergiomensing commented 1 year ago

Thanks @joerideg! It says I have to login to view the issue. Is this right?

machak commented 1 year ago

@sergiomensing you can't access the issue (it is part of an internal JIRA project)

joerideg commented 1 year ago

Hi, we just released 21.0.0. This reverts the async behavior we introduced in 18.