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

[client] js: Fetch public conf once from /config; embed.js: Preserve default config values #821

Merged
merged 9 commits into from
Jun 5, 2022

Conversation

ix5
Copy link
Member

@ix5 ix5 commented Mar 20, 2022

isso: js: embed.js: Preserve default config values

This way, Isso is not confusing users when a server config value overwrites a client default one, i.e. if the user has not actually attempted to overwrite it.

isso: js, py: Fetch public conf once from /config

This is an improvement on top of: 02f3ea0 "Have client read out shared settings from server (#311)"

So far, the config dict was being sent with the server response when fetching comments from the fetch() / (GET) endpoint.

Remove the config object from fetch() responses and instead let client user the newly created /config endpoint. (see #880)

Extend the JS client api file to fetch from the /config endpoint, which will only happen once via the fetchComments hook on page load.

EDIT: Have a couple of nasty race conditions right now, needs further work. Caused by multiple instances of config copying from default_config, where the templates might receive either a pre-copy or post-copy instance.

@ix5 ix5 added client (Javascript) client code and CSS feature labels Mar 20, 2022
@ix5 ix5 added this to the 0.13 milestone Mar 20, 2022
isso/js/app/config.js Outdated Show resolved Hide resolved
@ix5 ix5 force-pushed the client-fetch-config-once branch from c55c9cc to 706d1e7 Compare March 21, 2022 20:24
@ix5
Copy link
Member Author

ix5 commented Mar 21, 2022

cc @pellenilsson @brad, looking forward to your reviews. Hope you can rebase your changes on top of this.

@ix5 ix5 force-pushed the client-fetch-config-once branch 3 times, most recently from 2a88419 to ce3f12b Compare March 29, 2022 21:48
@ix5 ix5 force-pushed the client-fetch-config-once branch 2 times, most recently from 6af6e1d to 11f3e7c Compare April 23, 2022 20:26
@ix5
Copy link
Member Author

ix5 commented Apr 26, 2022

@Kerumen continuing discussion from #842

It seems that in development, window.Isso.fetchComments() reload the whole page. I have to dig more.

That seems strange to me. With the branch from #821, I don't see a refresh of the whole window. Both init() and fetchComments() now do exactly what they're supposed to.


But I have discovered another issue which is a race condition during the reading of the default config and fetching of the server config. See this branch with some logging. You need to refresh the page a couple of times to hit it.

comment:
Object { css: true, "css-url": null, "max-comments-top": "inf", "max-comments-nested": 5, "reveal-on-click": 5, avatar: true, "avatar-bg": "#f0f0f0", "avatar-fg": (8) […], vote: true, "vote-levels": null, … }
comment: gravatar
true
comment:
Object { css: true, "css-url": null, "max-comments-top": "inf", "max-comments-nested": 5, "reveal-on-click": 5, avatar: true, "avatar-bg": "#f0f0f0", "avatar-fg": (8) […], vote: true, "vote-levels": null, … }
    avatar: true
    "avatar-bg": "#f0f0f0"
    "avatar-fg": Array(8) [ "#9abf88", "#5698c4", "#e279a3", … ]
    css: true
    "css-url": null
    feed: false
    gravatar: false
    langs: Array(4) [ "en-US", "en", "en", … ]
    "max-comments-nested": 5
    "max-comments-top": "inf"
    "reply-notifications": false
    "reply-to-self": false
    "require-author": false
    "require-email": false
    "reveal-on-click": 5
    vote: true
    "vote-levels": null
    <prototype>: Object { … }

!!!
comment: gravatar
true
# should not be true!
!!!

Setting reply-to-self to false
Setting require-email to false
Setting require-author to false
Setting reply-notifications to false
Isso: Client value 'true' for setting 'gravatar' overridden by server value 'false'.
Since Isso version 0.12.6, 'data-isso-gravatar' is only configured via the server to keep client and server in sync
Setting gravatar to false

GET http://localhost:8080/demo/undefined
[HTTP/1.1 404 NOT FOUND 4ms]

Note conf.gravatar is supposed to be overriden to false because of the server config:

{
  "config": {
    "reply-to-self": false,
    "require-email": false,
    "require-author": false,
    "reply-notifications": false,
    "gravatar": false
  }
}

But in some cases (like the above), it'll result in some instances being set to true and later/earlier(?) evaluating to false.

This is due to some bad importing practices - a lot of code relies on config, templates or i18n but these get imported multiple times from multiple sources, and they're each carrying state instead of being purely functional. I'm not sure what the best way forward is here - have everything originate from embed.js and pass config as a parameter to each successive import/call?

My idea for how the flow should look like is this:

  • embed.js loaded
  • have default config
  • look for data-isso-* vars
  • look for endpoint, or fall back to script source
  • fetch config
  • use fetched config vars to overwrite default
  • initialize isso
  • render postbox
  • fetch comments
  • render comments

Further problems

  • Class vs instance vars
  • Multiple copies of e.g. config, i18n
  • Race conditions
  • Circular imports

@ix5 ix5 mentioned this pull request May 24, 2022
5 tasks
@ix5 ix5 force-pushed the client-fetch-config-once branch from 11f3e7c to b25b188 Compare May 24, 2022 00:30
ix5 added 9 commits June 5, 2022 18:40
This way, Isso is not confusing users when a server config
value overwrites a client default one, i.e. if the user
has not actually attempted to overwrite it.
This is an improvement on top of:
02f3ea0
"Have client read out shared settings from server (isso-comments#311)"

So far, the `config` dict was being sent with the server
response when fetching comments from the `fetch()` `/` (GET)
endpoint.

Remove the `config` object from `fetch()` responses and
instead let client user the newly created `/config` endpoint.

Extend the JS client api file to fetch from the `/config`
endpoint, which will only happen once via the
`fetchComments` hook on page load.
Check if something is ready, and if not, register self as
listener to be triggered once it is ready.

To be used by `embed.js` to coordinate between dependents of
the `/config` endpoint.
Those two functions need to wait for the config to be
fetched from the server. Utilize the newly introduced
`wait_for` helper mechanism to coordinate using registered
triggers instead of using timeout/retry.
Those do not need to wait for server config. By re-shuffling
the logic a bit, the `init` function can also bail out
earlier if no `#isso-thread` is found on page.
The `$('style#isso-style')` check was always returning
`null`. Simply use the plain id to fix that.
The `isso-thread-heading` class was added to `<h4>` element,
snapshots need to be updated to reflect that.
Commit 1e01a27
"views/comments, docs: Fetch feed config from server",
together with 748d78a
"contrib: isso-dev.cfg: Enable/prepare more testing options"
enabled the RSS/Atom feed on the server side and made the
client respect that. Update snapshot that now shows a link
to an Atom feed.
@ix5 ix5 force-pushed the client-fetch-config-once branch from b25b188 to 1d8e059 Compare June 5, 2022 17:49
@ix5
Copy link
Member Author

ix5 commented Jun 5, 2022

I am positive that by introducing a sort-of lock using wait_for, the races for config options should be fixed. Merging, can still be improved upon later.

@ix5 ix5 merged commit 628246c into isso-comments:master Jun 5, 2022
@ix5 ix5 deleted the client-fetch-config-once branch June 5, 2022 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client (Javascript) client code and CSS feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants