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

Fix for #121 #122

Closed
wants to merge 1 commit into from
Closed

Fix for #121 #122

wants to merge 1 commit into from

Conversation

klarstrup
Copy link
Contributor

This is the proof of concept fix for #121

I want to collect some feedback with regards to the element id namespacing and the general approach to access these JSON blobs. document.getElementById().innerHTML is used here but there are many options to select them(ids, classes, data attributes).

I'd also like to talk about line 45 and its surroundings now, which is pretty dirty. Doing what is basically abstracted eval'ing(but I didn't want to include this change immediately):

(out += `window['${key}'] = JSON.parse(document.getElementById('${idPrefix}${key}').innerHTML);`)

My suggestion is to replace this script tag with another JSON blob containing our keys and moving all the window rehydration logic over into kit/entry/browser.js, check out my approach to that over at https://github.com/klarstrup/kit/compare/json-window...klarstrup:json-window-clean?expand=1 and tell me what you think.

@klarstrup klarstrup closed this Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant