-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Added serviceworker and web-manifest #154
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give this a test. It LGTM.
@kylecarbs Before this gets merged in we have to decide if we want 'StaleWhileRevalidate' caching or 'NetworkFirst'. Also how long should the cache be valid?
NetworkFirst will try to pull from network but the request doest finish within a set amount of time, it will use the cache instead (if available).
StaleWhileRevalidate will jump to cache instantly if available, and will, once the network has calmed, download up to date files from the server into the cache for use next time the page is loaded.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I like StaleWhileRevalidate
. You should be able to test now, as both issues have been resolved!
Does this work as expected @creativeguy2013?
webbertakken
commented
Mar 15, 2019
LGTM
On a Chromebook with slow hardware this would bring a great performance increase compared to running the actual VS Code through the linux terminal while also keeping a native experience.
I guess the same would have to be applied to the /ssh
route on coder.com, for a split screen experience having both of these open. I haven't found any reference in this repository, how would this be handled?
LGTM, this would increase performance on slower networks as well.
I am checking if everything works as expected now, was very busy the last few weeks. Sorry for the long wait.
The service worker now properly loads. There are a few important details though. The service worker only initializes when a TRUSTED SSL certificate is used. They also don't work on non HTTPS connections. Everything else will work normally though.
I benchmarked everything a little. The service worker reduces bandwidth required by client and server by about 650x after the first request. Depending on what you do in the first session this may be higher or lower as icons are still loaded gradually. We might want to preload all icons in the background later.
You can now install code-server client as a PWA on iOS (???), Android (chrome), Windows (chrome), Mac (chrome) and Linux (chrome).
@Hasan123987 Please stop typing in this issue unless you have questions related to this issue.
Any reason this hasn't been merged yet? Everything should be ready now.
andreimc
commented
Mar 27, 2019
👍 to get this merged
@kylecarbs @multishifties feedback this issue pls
andreimc
commented
Mar 29, 2019
I have been running this and I get a warning from the monaco editor:
t.logOnceWebWorkerWarning @ simpleWorker.ts:35
t._getOrCreateWorker @ editorWorkerServiceImpl.ts:355
t._getProxy @ editorWorkerServiceImpl.ts:363
t._withSyncedResources @ editorWorkerServiceImpl.ts:378
t.computeLinks @ editorWorkerServiceImpl.ts:403
(anonymous) @ editorWorkerServiceImpl.ts:65
Promise.then (async)
provideLinks @ editorWorkerServiceImpl.ts:65
(anonymous) @ getLinks.ts:75
d @ getLinks.ts:74
(anonymous) @ links.ts:249
l @ async.ts:23
(anonymous) @ links.ts:249
(anonymous) @ tslib.es6.js:97
(anonymous) @ tslib.es6.js:78
(anonymous) @ tslib.es6.js:71
u @ tslib.es6.js:67
e.beginCompute @ links.ts:238
e.onModelChanged @ links.ts:226
(anonymous) @ links.ts:207
e.fire @ event.ts:584
t.setModel @ codeEditorWidget.ts:421
(anonymous) @ textFileEditor.ts:153
Promise.then (async)
(anonymous) @ textFileEditor.ts:135
Promise.then (async)
t.setInput @ textFileEditor.ts:134
t.doSetInput @ editorControl.ts:178
t.openEditor @ editorControl.ts:70
t.doShowEditor @ editorGroupView.ts:792
t.restoreEditors @ editorGroupView.ts:420
t @ editorGroupView.ts:144
t.create @ types.ts:169
e._createInstance @ instantiationService.ts:104
e.createInstance @ instantiationService.ts:69
t.createFromSerialized @ editorGroupView.ts:59
t.doCreateGroupView @ editorPart.ts:515
fromJSON @ editorPart.ts:894
t.deserializeNode @ grid.ts:460
t.deserializeNode @ grid.ts:453
t.deserialize @ grid.ts:489
t.doCreateGridControlWithState @ editorPart.ts:888
t.doCreateGridControlWithPreviousState @ editorPart.ts:850
t.doCreateGridControl @ editorPart.ts:822
t.createContentArea @ editorPart.ts:799
t.create @ part.ts:53
hn.createEditorPart @ workbench.ts:1280
hn.renderWorkbench @ workbench.ts:1236
hn.doStartup @ workbench.ts:439
hn.startup @ workbench.ts:385
(anonymous) @ main.ts:130
Promise.then (async)
(anonymous) @ main.ts:109
Promise.then (async)
n.open @ main.ts:107
t.main @ main.ts:333
(anonymous) @ workbench.ts:199
(anonymous) @ tslib.es6.js:97
(anonymous) @ tslib.es6.js:78
(anonymous) @ tslib.es6.js:71
u @ tslib.es6.js:67
e.initialize @ workbench.ts:167
(anonymous) @ client.ts:28
(anonymous) @ tslib.es6.js:97
(anonymous) @ tslib.es6.js:78
(anonymous) @ tslib.es6.js:71
u @ tslib.es6.js:67
(anonymous) @ client.ts:20
(anonymous) @ client.ts:92
(anonymous) @ tslib.es6.js:97
(anonymous) @ tslib.es6.js:78
s @ tslib.es6.js:68
Promise.then (async)
c @ tslib.es6.js:70
(anonymous) @ tslib.es6.js:71
u @ tslib.es6.js:67
e.task @ client.ts:80
r.initialize @ client.ts:20
e @ client.ts:57
r @ client.ts:18
(anonymous) @ client.ts:118
(anonymous) @ client.ts:118
r @ bootstrap:63
(anonymous) @ index.ts:1
r @ bootstrap:63
(anonymous) @ index.ts:2
r @ bootstrap:63
(anonymous) @ bootstrap:195
(anonymous) @ bootstrap:195
simpleWorker.ts:37 Cannot find module './workerMain.js'
andreimc
commented
Mar 29, 2019
looking at this: https://github.com/Microsoft/monaco-editor-webpack-plugin -> I think it should be added so monaco uses service workers
@andreimc does that warning break functionality?
Once conflicts are in I'd be happy to merge.
andreimc
commented
Apr 3, 2019
@kylecarbs, it doesn’t. I have a PR ready to actually enable Monaco via service worker. But it needs this to go in first.
andreimc
commented
Apr 3, 2019
I fixed conflicts and added MonacoWebpackPlugin on this PR @kylecarbs #411
I think we should merge this PR now and then add the monaco worker via a separate PR from master. Then the two PRs can stay split.
Related: #411 (comment)
Service workers !== web workers
* sw/master: Changed single to double quotes Serviceworker now properly loads added caching added assets to individual module folders actually fixed images i think fixed image link added deps in package.json Added serviceworker and manifest.json
andreimc
commented
Apr 4, 2019
@creativeguy2013 I have added a PR on your master to fix conflicts if you merge that in this can go in. Yeah agreed the web workers can be addressed later
Fixes your conflicts so 154 can be merged
Apologies for nit-picking: could we revert the indentation back to tabs, and use tabs for any newly added code?
Do we need to commit the logo twice? Could we keep it perhaps in just the web
directory and use the root
variable to reference it via absolute path? Edit: will have to do this (or create a third copy in server
) since when I run yarn start
it's trying to read packages/server/assets/logo.png
which doesn't exist.
Do we need to commit the logo twice? Could we keep it perhaps in just the
web
directory and use theroot
variable to reference it via absolute path? Edit: will have to do this (or create a third copy inserver
) since when I runyarn start
it's trying to readpackages/server/assets/logo.png
which doesn't exist.
I added a third copy in server. Not quite sure what you mean with root
variable. If you elaborate on this I can implement it.
About only registering the service worker for production: this seems to work fine now when run in the codercom/code-server/master. It was indeed the NODE_ENV in the Dockerfile.
Everything should be good to go now, so if there are no there comments you can merge.
* Added serviceworker and manifest.json * added deps in package.json * fixed image link * actually fixed images i think * added assets to individual module folders * added caching * Serviceworker now properly loads * Changed single to double quotes * Update lock * moved the service worker back into prod only * removed sw from general * changed background color of splash screen * added logo to server * centralized logo into single assets folder
Uh oh!
There was an error while loading. Please reload this page.
I added a basic web-maifest (using webpack-pwa-manifest) and a workbox based service-worker (using workbox-webpack-plugin). The service worker gets generated automatically by workbox and gets auto injected by webpack. If more advanced service-worker functionality is needed this can still be achieved with workbox-webpack-plugin. More info about this can be found on https://developers.google.com/web/tools/workbox/modules/workbox-webpack-plugin.
Workbox is currently set to cache all resources created by webpack (default). This can be changed with some options. The service-worker is only injected in prod, not dev (someone should test this please).
I just put in some basic stuff into the manifest, you might want to change the name or description. I did not know where to put the logo image for the manifest so I created a new assets folder. You might want to move this too.