Fixed build for usage without subdomain #806

sattellite · 2020-11-02T06:50:25Z

I installed remark42 in the easiest way from the docker. Configured nginx to use app without subdomain. It works okay for comments but widgets not working as expected. Widgets use it own config with host as window.location.origin which returns the fqdn only.

This can be fixed on server side by multiple sed commands in container like this docker exec -it remark42 sed -i "s|host: window.location.origin|host: 'https://b.sattellite.me/comments/'|g" /srv/web/last-comments.html

I prepared fix to code for exclude this problem in future:

Replaced host value from window.location.origin to "REMARK_URL" || window.location.origin.
Changed init script for container to replace forced site_id by 'remark' to environment SITE_ID.

docker-init.sh

akellbl4 · 2020-11-02T08:51:30Z

docker-init.sh

+
+if [ -n "${SITE_ID}" ]; then
+  #replace "site_id: 'remark'" by SITE_ID
+  se -i "s|'remark'|'${SITE_ID}'|g" /srv/web/*.html


Also, I think SITE_ID could be passed via env vars on build time without replacing it after.

Yes, if I will build my own docker image this pull request not needed. But I use public docker imageumputun/remark42 and this changes will fix it for usage without subdomain.

Replacing remark could cause side-effects like replacing this word in unexpected palaces.
I could suggest putting unique value or template like expression to places that we want to replace.

Something like {% SITE_ID %} and comment about how it works.
The same we should do for replacing REMARK_URL

Yes, current solution is dirty hack. And site_id is hard coded. But this script called only inside docker container. Would not this solution break the usage without docker on bare metal?

If I will add expressions like {% SITE_ID %} and {% REMARK_URL %} it will keep the same after npm run build. And replace expressions to correct values will require postbuild script with sed commands.

I don't know how to fix this correctly, given what I said above.
Or am I wrong?

Yep, you are right. And now we have the issue in bin build. With this PR we will fix the issue only for docker.
Let's use your fix and I will think about how to fix it in all ways for all builds.

umputun · 2020-11-02T09:23:15Z

confused. How this even related to subdomain configuration vs without subdomain configuration? Either way REMARK_URL has to be passed in or UI it won't be able to communicate with the server.

Widgets use it own config with host as window.location.origin which returns the fqdn only.

I don't get it. @akellbl4 correct if I wrong, but I think window.location.origin is not used to discover the server. I don't see how this can be the case and how window.location.origin can be used as a substitution for REMARK_URL

akellbl4 · 2020-11-02T09:51:26Z

@umputun, I forgot to say that I made review only from code point of view. And I want to reproduce this bug and check how this PR will fix it.
window.location.origin takes current host name where document is served. It means this variable should represent current remark host.

sattellite · 2020-11-02T10:19:55Z

This fix affects the pages of widgets, when directly accessing them.

/srv/web contains static html files for widgets: comments.html, last-comments.html, counter.html and demo page index.html.
This pages contain mostly identical inline js-config for remark42:

var remark_config = {
    site_id: 'remark',
    host: window.location.origin,
    components: ['last-comments'] // or other 
};

And this config is used to append widget scripts:

(function(c) {
  for (var i = 0; i < c.length; i++) {
    var d = document,
      s = d.createElement('script');
    s.src = remark_config.host + '/web/' + c[i] + '.js';
    (d.head || d.body).appendChild(s);
  }
})(remark_config.components || ['embed']);

remark_config.host contains origin (fqdn) instead of correct REMARK_URL. Generates incorrect url for component => script not loaded => widget don't working.

Current behavior for REMARK_URL=https://remark.example.com:

remark_config.host will be equal https://remark.example.com
page https://remark.example.com/web/last-comments.html will load https://remark.example.com/web/last-comments.js
all work as expected

Current behavior for REMARK_URL=https://example.com/remark:

remark_config.host will be equal https://example.com
page https://example.com/remark/web/last-comments.html will try load https://example.com/web/last-comments.js
empty page and 404 error in console

My patches force to set correct REMARK_URL and SITE_ID to relevant variables:

var remark_config = {
    site_id: 'mysiteid',
    host: 'https://example.com/remark' || window.location.origin,
    components: ['last-comments'] // or other 
};

And sed is needed to replace constant values that was used when building docker image.

umputun · 2020-11-02T21:00:31Z

thx for the detailed explanation.

sed we currently doing is not something I proud of. If possible it will be nice to resolve the issue without adding more seds.

sattellite · 2020-11-03T06:27:23Z

sed is simple solution in my opinion. Other solutions make it more difficult:

on backend side serve as templates, not static files. Fully generate all html and js with webpack to correct files for html/template. All sed will not be needed.
on backend side add api route to get current host and bucket (no new sed, old sed will remain)
on web server(nginx, caddy, etc.) side replace strings. Ugly and difficult, but sed will be not needed.

sattellite · 2020-11-03T07:50:50Z

And if create serving templates instead static files it will be step to solve style customization issue #5

akellbl4 · 2020-11-03T10:21:46Z

@sattellite as I said in comments all of these things could be done only with passing env vars via webpack to templates and js.
Also as I understood it's not a critical bug because it doesn't work only on the demo page.

UPD: I've checked our building process again. And if we want to handle it via webpack we need to do it on the build stage and variables pass to container on run time. It means we could change these vars only by sed or be serving them and replace it by go.

akellbl4 · 2020-11-03T10:37:26Z

This bug relative to the PR.

sattellite · 2020-11-03T10:45:17Z

UPD: I've checked our building process again. And if we want to handle it via webpack we need to do it on the build stage and variables pass to container on run time. It means we could change these vars only by sed or be serving them and replace it by go.

Agree with you. Serving with go web-server also can help solve other problems, as I said upper.

Does this mean that I need to add patches to process this with go?

akellbl4 · 2020-11-04T16:22:32Z

@umputun wdyt? Using sed means we have some bad practice but an easier way to serve HTML. Serve templates with transformation in go means cleaner code but more complexity in this place.

I prefer to stay with sed for now and think about how we can handle it in some other way because I don't want to bring more complexity.

umputun · 2020-11-06T08:32:06Z

yeah, agree. will keep sed for now

akellbl4 · 2020-11-08T18:07:55Z

frontend/webpack.config.js

@@ -180,21 +180,33 @@ module.exports = () => ({
    new Html({
      template: path.resolve(__dirname, 'index.ejs'),
      inject: false,
+      templateParameters: {


Sorry for mixing you up. This way is absolutely useless for us and we can remove these params.

This option allows use REMARK_URL instead of htmlWebpackPlugin.options.REMARK_URL

https://github.com/jantimon/html-webpack-plugin#:~:text=templateParameters

Yep, I understand. But it's useless because we should to replace strings only on running time with passed vars to env and this template compilation will be done on building stage.

akellbl4 · 2020-11-08T18:09:43Z

frontend/last-comments.ejs

@@ -25,7 +25,7 @@
    <script>
      var remark_config = {
        site_id: 'remark',
-        host: window.location.origin,
+        host: '<%= REMARK_URL %>' || window.location.origin,


REMARK_URL is required param. It means this expression will always return the first value and we don't need to keep window.location.origin

REMARK_URL is required and window.location.origin is not needed. umputun#806 (comment)

umputun

thx

REMARK_URL is required and window.location.origin is not needed. #806 (comment)

sattellite added 2 commits November 2, 2020 08:57

use REMARK_URL in frontend templates

6807aaf

Replace BASE_URL constant by REMARK_URL in html

5ffa7cc

sattellite requested a review from umputun as a code owner November 2, 2020 06:50

Replace predefined site_id by SITE_ID

b2b00f9

umputun requested review from akellbl4 and paskal November 2, 2020 07:14

akellbl4 reviewed Nov 2, 2020

View reviewed changes

docker-init.sh Show resolved Hide resolved

akellbl4 reviewed Nov 2, 2020

View reviewed changes

sattellite requested a review from akellbl4 November 2, 2020 09:12

akellbl4 reviewed Nov 8, 2020

View reviewed changes

sattellite added 2 commits November 9, 2020 16:30

Simplified js expression in templates

05cebb5

REMARK_URL is required and window.location.origin is not needed. umputun#806 (comment)

Simplified definition of REMARK_URL to templates in webpack config

7e1f004

akellbl4 approved these changes Nov 10, 2020

View reviewed changes

akellbl4 mentioned this pull request Nov 10, 2020

Right way to pass SITE_ID and REMARK_URL to templates #813

Open

umputun approved these changes Nov 10, 2020

View reviewed changes

umputun merged commit 60f554f into umputun:master Nov 10, 2020

umputun pushed a commit that referenced this pull request Nov 10, 2020

Simplified js expression in templates

abe3b0d

REMARK_URL is required and window.location.origin is not needed. #806 (comment)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed build for usage without subdomain #806

Fixed build for usage without subdomain #806

sattellite commented Nov 2, 2020

akellbl4 Nov 2, 2020

sattellite Nov 2, 2020

akellbl4 Nov 8, 2020

sattellite Nov 9, 2020 •

edited

Loading

akellbl4 Nov 10, 2020

umputun commented Nov 2, 2020

akellbl4 commented Nov 2, 2020 •

edited

Loading

sattellite commented Nov 2, 2020 •

edited

Loading

umputun commented Nov 2, 2020

sattellite commented Nov 3, 2020

sattellite commented Nov 3, 2020

akellbl4 commented Nov 3, 2020 •

edited

Loading

akellbl4 commented Nov 3, 2020

sattellite commented Nov 3, 2020 •

edited

Loading

akellbl4 commented Nov 4, 2020

umputun commented Nov 6, 2020

akellbl4 Nov 8, 2020 •

edited

Loading

sattellite Nov 9, 2020

akellbl4 Nov 9, 2020

akellbl4 Nov 8, 2020

umputun left a comment

Fixed build for usage without subdomain #806

Fixed build for usage without subdomain #806

Conversation

sattellite commented Nov 2, 2020

akellbl4 Nov 2, 2020

Choose a reason for hiding this comment

sattellite Nov 2, 2020

Choose a reason for hiding this comment

akellbl4 Nov 8, 2020

Choose a reason for hiding this comment

sattellite Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

akellbl4 Nov 10, 2020

Choose a reason for hiding this comment

umputun commented Nov 2, 2020

akellbl4 commented Nov 2, 2020 • edited Loading

sattellite commented Nov 2, 2020 • edited Loading

umputun commented Nov 2, 2020

sattellite commented Nov 3, 2020

sattellite commented Nov 3, 2020

akellbl4 commented Nov 3, 2020 • edited Loading

akellbl4 commented Nov 3, 2020

sattellite commented Nov 3, 2020 • edited Loading

akellbl4 commented Nov 4, 2020

umputun commented Nov 6, 2020

akellbl4 Nov 8, 2020 • edited Loading

Choose a reason for hiding this comment

sattellite Nov 9, 2020

Choose a reason for hiding this comment

akellbl4 Nov 9, 2020

Choose a reason for hiding this comment

akellbl4 Nov 8, 2020

Choose a reason for hiding this comment

umputun left a comment

Choose a reason for hiding this comment

sattellite Nov 9, 2020 •

edited

Loading

akellbl4 commented Nov 2, 2020 •

edited

Loading

sattellite commented Nov 2, 2020 •

edited

Loading

akellbl4 commented Nov 3, 2020 •

edited

Loading

sattellite commented Nov 3, 2020 •

edited

Loading

akellbl4 Nov 8, 2020 •

edited

Loading