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

serve config.json statically instead of bundling it #1516

Merged
merged 1 commit into from
Jun 6, 2016

Conversation

vith
Copy link
Contributor

@vith vith commented May 15, 2016

issue #1344

If you don't like the use of es7 features and/or fetch, let me know. I just wanted to throw something together quickly so I could package vector-web for the Arch Linux AUR.

@vith
Copy link
Contributor Author

vith commented May 15, 2016

By the way, I developed and tested my changes against v0.6.1 before rebasing them on develop as per https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst#how-to-contribute. The develop branch is broken at least in my environment, with or without this PR, so I haven't been able to test my changes on the branch the PR merges against.

@ara4n
Copy link
Member

ara4n commented May 15, 2016

cool - thanks for this! :) i am a bit dubious about mixing the promises libraries (vector has enough deps already) but the change in general looks good. I'm on mobile atm but will merge it in using the existing http client & promise libs later today. And very cool to have vector pkged for Arch - thanks!

@ara4n
Copy link
Member

ara4n commented May 15, 2016

dev shouldn't be broken, btw - did you pull in the dev branches of matrix-react-sdk and matrix-js-sdk?

@DoubleMalt
Copy link

Thanks @vith!

@ara4n
Copy link
Member

ara4n commented May 18, 2016

we haven't forgotten this - just running low on tuits. this will close #1344

@DoubleMalt
Copy link

If there is anything I can do to help, tell me :)

@vith
Copy link
Contributor Author

vith commented May 24, 2016

Sorry, you're right about develop branch working, I hadn't manually replaced the npm modules.

@vith
Copy link
Contributor Author

vith commented May 25, 2016

I've updated the PR to use q and browser-request instead of the web standards polyfills. My bad for not noticing those deps already existed on the matrix sdk libs. Let me know if you want any other changes.

@ara4n
Copy link
Member

ara4n commented May 25, 2016

ah, thanks; that looks much better. @dbkr can you take a look please once you are back in vectorland?

@dbkr
Copy link
Member

dbkr commented Jun 2, 2016

Is there any risk that the request for the config file could fail and leave the user stuck with a broken vector (could a browser fetch the page entirely from cache and then fail to get the config if you had no internet connection?)

Also this has just been broken by us adding a require('config.json') tocomponents/views/settings/Notifications.js` so we should fix that first, then merge this.

We could probably also remove the json webpack loader too as I think this is the only thing using it.

@vith
Copy link
Contributor Author

vith commented Jun 4, 2016

@dbkr That worry crossed my mind as well, but since it's a static file I figured the chance of config.json failing to load was the same as the risk for static css and js assets, for which there is generally no real failure handling (retry or UI). For this reason I didn't bother to handle request failure.

However, an argument against that could be that the request for config.json is not a sibling of the requests for the other static assets, but rather a child of them. This means there's an additional round-trip-time delay, so maybe it would make sense to display a loading message?

HTTP/2 Server Push could be used to make config.json act more like a sibling asset to the document as far as request timing goes. <script type="module"> would at least make it a sibling of the javascript bundle, though I don't think webpack's module loader runtime supports that.

Anyway, if it's desired I can change this PR to immediately render a "loading" message and/or retry failed requests and/or render a "loading failed" message.

@vith
Copy link
Contributor Author

vith commented Jun 4, 2016

could a browser fetch the page entirely from cache and then fail to get the config if you had no internet connection?

That's a good point that I didn't really address, a webpack bundle could use long-term caching by including a hash in the filename. Wouldn't the document have to be uncached so that new versions of the document can get loaded that point to new webpack bundles though?

I suppose there may be a possibility of falling back to a cached version if the request fails. I would have to do some research into that.

@dbkr
Copy link
Member

dbkr commented Jun 6, 2016

Agree with you on the failure modes for fetching the config: we can easily add a retry down the line if it turns out to be necessary. I'm going to try & merge this although will have to make sure we haven't broken it in the meantime by adding require('config.json')s.

@dbkr
Copy link
Member

dbkr commented Jun 6, 2016

Merging now that I've fixed the place that requires config.json (f9aaf7d).

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.

4 participants