Fix to work without errors in Nuxt Universal #723

patarapolw · 2020-05-25T15:31:49Z

As a solution to #722 I cloned and edited the repo.

In embed.ts,
- Replace !. with ?. so it will not throw error ... of undefined.
- Add destroy() to window.Remark42, which actually removes global listeners (on window and document)
- Prevent duplication of Remark42 Iframe with

  const oldIframe = document.querySelector('iframe[title=Remark42]') as HTMLIFrameElement;
  if (oldIframe) {
    return oldIframe;
  }

Also, in production, instead of your init function https://github.com/umputun/remark42#comments, I moved it inside a function and add a typing for TypeScript. -- remark42.ts and it is implemented here -- PostFull.vue

In order to test in production, I deploy it to Google Compute Engine and Docker Hub with patarapolw/remark42, as can be seen at

https://remark42.polv.cc/web -- Remark42 hosting
https://www.polv.cc -- Nuxt with Remark42

umputun · 2020-05-25T17:41:16Z

I'm not really sure what we supposed to see, but both links from the message above don't work. One returns "not found" (maybe you meant https://remark.polv.cc/web/ ?) and the second "Error: Server Error"

patarapolw · 2020-05-25T18:08:22Z

@umputun Sorry, the website was down for some reasons (Google App Engine throws no log.) But it is back now.

Mavrin

For SPA we could create api like:

const instance = window.REMARK42.createInstance({
  host: "host",
  site_id: "siteId",
  url: "url",
  max_shown_comments: 50,
  theme: "light",
  page_title: "test",
  node: DOMNode,
  locale: "en",
});
// instance.changeTheme();
// instance.destroy();

frontend/app/embed.ts

Mavrin · 2020-05-26T19:49:28Z

frontend/app/embed.ts

-        this.back!.setAttribute('data-animation', '');
-        this.node!.setAttribute('data-animation', '');
-        this.iframe!.focus();
+        this.back?.setAttribute('data-animation', '');


node should be always exist. Nullish operator should not be use for these cases. If we get null that mean that destroy doesn't work properly. It can create memory leak.

I see, so it would probably become,

if (!this.node) { this.node = document.createElement('div'); ... } if (!this.back) { this.back = document.createElement('div'); ... } this.iframe = iframe; ... setTimeout(() => { this.back!.setAttribute('data-animation', ''); this.node!.setAttribute('data-animation', ''); iframe.focus(); }, 400);

paskal · 2020-06-14T09:37:13Z

Gentle ping for @patarapolw to change the code after the review.

patarapolw · 2020-06-14T11:04:08Z

Do you need some kind of commitiquette, like commitizen or commitlint?

Also, pre-commit Husky hooks should entail more IMO, rather than rely on pre-push.

~/projects/remark42(master*) » git commit                                                                                                    patarapolw@192
husky > pre-commit (node v12.16.3)
  ↓ Stashing changes... [skipped]
    → No partially staged files found...
  ✔ Running tasks...
husky > post-commit (node v12.16.3)
[master 6358acd2] fix onDestroy-related methods
 1 file changed, 16 insertions(+), 11 deletions(-)
------------------------------------------------------------------------------------------------------------------------------------------------------------
~/projects/remark42(master) » git push origin master                                                                                         patarapolw@192
husky > pre-push (node v12.16.3)

> remark-ui@0.1.0 check /Users/patarapolw/projects/remark42/frontend
> cross-env NODE_ENV=production npm run build && run-p check:*


> remark-ui@0.1.0 build /Users/patarapolw/projects/remark42/frontend
> webpack --mode production

NODE_ENV = production
REMARK_ENV = https://demo.remark42.com

akellbl4 · 2020-06-15T07:56:29Z

LGTM, but I would ask @Mavrin to revise.

patarapolw · 2020-06-18T07:39:19Z

BTW, does not work in GatsbyJS, but OK in NextJS (and of course NuxtJS).

A reproducible repo is here -- https://github.com/patarapolw/jpdiary/tree/gatsby

Mavrin · 2020-06-18T09:28:06Z

@patarapolw. Thank you for reproducible example. Did you read my comment?

For SPA we could create api like:

const instance = window.REMARK42.createInstance({
  host: "host",
  site_id: "siteId",
  url: "url",
  max_shown_comments: 50,
  theme: "light",
  page_title: "test",
  node: DOMNode,
  locale: "en",
});
// instance.changeTheme();
// instance.destroy();

I think if you impent this all should work as expected. You need to call this code when page is mount and unmount.

patarapolw · 2020-06-22T15:04:34Z

@patarapolw. Thank you for reproducible example. Did you read my comment?
For SPA we could create api like:
const instance = window.REMARK42.createInstance({
  host: "host",
  site_id: "siteId",
  url: "url",
  max_shown_comments: 50,
  theme: "light",
  page_title: "test",
  node: DOMNode,
  locale: "en",
});
// instance.changeTheme();
// instance.destroy();
I think if you impent this all should work as expected. You need to call this code when page is mount and unmount.

Ok, it seems that I have to frame the createInstance function. But, what is the best way to test it out without git commit and git push, that is relying on git hooks?

Merge branch 'master' of github.com:umputun/remark42

…fore try to attach node

Mavrin · 2020-06-23T06:25:28Z

You can run all test by npx run-p check lint.
I will look PR today.

frontend/app/embed.ts

Mavrin

Looks good. Could you please revert all replaced !. back?

patarapolw · 2020-06-25T15:40:27Z

@Mavrin OK, but in theory, I am not sure when iframe.contentWindow will be null?

Mavrin · 2020-06-25T15:46:00Z

@Mavrin OK, but in theory, I am not sure when iframe.contentWindow will be null?

If it is possible. We should add this check. Could you describe case when it is null?

Mavrin · 2020-06-25T16:04:51Z

Lgtm.
Did you try run current solution on your example?
Thank you.

akellbl4 · 2020-06-25T19:33:03Z

@patarapolw good job, would you be so kind to add documentation for this feature? And yes, as @Mavrin asked, it would be great to check working example.

patarapolw · 2020-06-27T00:09:30Z

For the example, where I really used, see my website -- https://www.polv.cc.

I also made a much simpler example -- https://github.com/patarapolw/remark42-spa (will update in there.)

patarapolw · 2020-06-27T03:54:09Z

frontend/app/embed.ts

    iframe.remove();
  }

+  // TODO: These do not appear in Chrome DevTools


I have this unresolved problem. Not really a problem to me, but may affect backward compatibility...

As I see API appears in DevTools

gvlasov · 2020-08-10T12:19:33Z

@patarapolw @umputun iframe reusing introduced in this PR breaks the possibility of having multiple comment sections on the same page. In this PR, iframe reuse is said to be for "Preventing duplication of Remark42 Iframe", but that definitely shouldn't be remark42's responsibility. It should be handled by client's component in frontend framework that uses remark42.

With current master, all that is needed to fix remark42 for multiple comment sections is changing

  const iframe =
    (document.querySelector('iframe[title=Remark42]') as HTMLIFrameElement) ||
    createFrame({ host: BASE_URL, query, __colors__: config.__colors__ });

to

  const iframe =
    createFrame({ host: BASE_URL, query, __colors__: config.__colors__ });

gvlasov · 2020-08-10T12:34:14Z

A small correction: that's definintely not the only thing preventing multiple comment sections from working on the same page, but changing that is required if multiple comment sections are ever to be implemented (I'm probably going to be working on it soon and I will provide a PR)

allow manual init and destroy for use with Nuxt

cdcbc48

patarapolw requested a review from umputun as a code owner May 25, 2020 15:31

paskal requested review from akellbl4, Mavrin and Reeywhaar May 25, 2020 15:46

paskal added the frontend label May 25, 2020

Mavrin requested changes May 26, 2020

View reviewed changes

fix onDestroy-related methods

6358acd

patarapolw requested a review from Mavrin June 14, 2020 11:13

avoid global scope, and use function scope instead

855eea0

patarapolw added 3 commits June 22, 2020 22:27

add createInstance function to window.REMARK42

3bb9d33

Update to upstream counterpart

3e63f0a

Merge branch 'master' of github.com:umputun/remark42

1. allow DOMNode to be put in remark_config 2. check remark_config be…

34b9e4c

…fore try to attach node

move createInstance function outside

41ff0ba

Mavrin reviewed Jun 24, 2020

View reviewed changes

frontend/app/embed.ts Outdated Show resolved Hide resolved

frontend/app/embed.ts Outdated Show resolved Hide resolved

frontend/app/embed.ts Outdated Show resolved Hide resolved

Mavrin requested changes Jun 24, 2020

View reviewed changes

frontend/app/embed.ts Outdated Show resolved Hide resolved

update embed.ts

2798d3e

Mavrin requested changes Jun 25, 2020

View reviewed changes

avoid ?.

1012c4f

Mavrin previously approved these changes Jun 25, 2020

View reviewed changes

📚 Docs: doc on how to make it work with SPAs

d36784d

patarapolw dismissed Mavrin’s stale review via d36784d June 26, 2020 05:51

patarapolw added 3 commits June 26, 2020 12:57

📚 Docs: fix spa.md to be more flexible

9911a57

✨ Feat: add REMARK42::ready event

adc174f

remove nuxt-specific terminologies

0794c6f

patarapolw added 2 commits June 27, 2020 10:46

tell MutationObserver to disconnect on destroy

8a8c60f

update docs/spa.md

7a8c387

patarapolw commented Jun 27, 2020

View reviewed changes

akellbl4 approved these changes Jul 10, 2020

View reviewed changes

umputun merged commit 5d6729d into umputun:master Jul 31, 2020

paskal added this to the v1.7 milestone Jan 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to work without errors in Nuxt Universal #723

Fix to work without errors in Nuxt Universal #723

patarapolw commented May 25, 2020 •

edited

Loading

umputun commented May 25, 2020

patarapolw commented May 25, 2020

Mavrin left a comment •

edited

Loading

Mavrin May 26, 2020

patarapolw Jun 14, 2020 •

edited

Loading

paskal commented Jun 14, 2020

patarapolw commented Jun 14, 2020 •

edited

Loading

akellbl4 commented Jun 15, 2020

patarapolw commented Jun 18, 2020

Mavrin commented Jun 18, 2020

patarapolw commented Jun 22, 2020 •

edited

Loading

Mavrin commented Jun 23, 2020

Mavrin left a comment

patarapolw commented Jun 25, 2020

Mavrin commented Jun 25, 2020

Mavrin commented Jun 25, 2020

akellbl4 commented Jun 25, 2020

patarapolw commented Jun 27, 2020

patarapolw Jun 27, 2020

akellbl4 Jul 10, 2020 •

edited

Loading

gvlasov commented Aug 10, 2020 •

edited

Loading

gvlasov commented Aug 10, 2020

Fix to work without errors in Nuxt Universal #723

Fix to work without errors in Nuxt Universal #723

Conversation

patarapolw commented May 25, 2020 • edited Loading

umputun commented May 25, 2020

patarapolw commented May 25, 2020

Mavrin left a comment • edited Loading

Choose a reason for hiding this comment

Mavrin May 26, 2020

Choose a reason for hiding this comment

patarapolw Jun 14, 2020 • edited Loading

Choose a reason for hiding this comment

paskal commented Jun 14, 2020

patarapolw commented Jun 14, 2020 • edited Loading

akellbl4 commented Jun 15, 2020

patarapolw commented Jun 18, 2020

Mavrin commented Jun 18, 2020

patarapolw commented Jun 22, 2020 • edited Loading

Mavrin commented Jun 23, 2020

Mavrin left a comment

Choose a reason for hiding this comment

patarapolw commented Jun 25, 2020

Mavrin commented Jun 25, 2020

Mavrin commented Jun 25, 2020

akellbl4 commented Jun 25, 2020

patarapolw commented Jun 27, 2020

patarapolw Jun 27, 2020

Choose a reason for hiding this comment

akellbl4 Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

gvlasov commented Aug 10, 2020 • edited Loading

gvlasov commented Aug 10, 2020

patarapolw commented May 25, 2020 •

edited

Loading

Mavrin left a comment •

edited

Loading

patarapolw Jun 14, 2020 •

edited

Loading

patarapolw commented Jun 14, 2020 •

edited

Loading

patarapolw commented Jun 22, 2020 •

edited

Loading

akellbl4 Jul 10, 2020 •

edited

Loading

gvlasov commented Aug 10, 2020 •

edited

Loading