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

Add support for indexeddb sync in webworker #3578

Merged
merged 4 commits into from Apr 7, 2017

Conversation

@dbkr
Copy link
Member

dbkr commented Apr 6, 2017

Provides a worker script for the js-sdk indexeddb webworker mode (matrix-org/matrix-js-sdk#412) and enables it using matrix-org/matrix-react-sdk#792

#3252

// However, this still pulls in all of the js-sdk and we only use a tiny fraction
// of it. It also causes an Olm error to appear because we don't have an Olm in scope.
// Instead, we do this:
import IndexedDbStoreWorker from 'matrix-js-sdk/lib/store/indexeddb-remote-worker';

This comment has been minimized.

Copy link
@Kegsay

Kegsay Apr 7, 2017

Contributor

Please don't advocate this style. I understand why you're doing it this case, but it prevents us from changing the position of files in the JS SDK. I don't believe we do pull in all of the JS SDK because of the optimisation pass which babel goes through in production mode. We should fix the Olm error rather than work around it like this.

It sounds like it will work if you do it properly, so please just do that.

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 7, 2017

Author Member

Sort of - the optimisation may be able to remove the unreferenced classes etc. but anything that's run in the root of the script (like the Olm detection or polyfills) can't be removed.

How about a compromise: I've moved the worker out to a separate import in src. It still needs to be imported via matrix-js-sdk/lib/indexeddb-worker (because you can only have one default) but it keeps the worker separate from everything else because it is logically separate, ie. you'd never use the worker class and the rest of the SDK in the same script. We're still importing via an import script rather than reaching right into the guts of the js-sdk though.

This comment has been minimized.

Copy link
@Kegsay

Kegsay Apr 7, 2017

Contributor

SGTM, thank you.

<% for (var i=0; i < htmlWebpackPlugin.files.js.length; i++) {
// Not a particularly graceful way of not putting the indexeddb worker script
// into the main page
if (htmlWebpackPlugin.files.js[i].endsWith('indexeddb-worker.js')) {

This comment has been minimized.

Copy link
@Kegsay

Kegsay Apr 7, 2017

Contributor

I'm fairly sure Babel doesn't get applied to this file. If it doesn't, then you cannot use endsWith as that is ES6 only :( https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/String/endsWith

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 7, 2017

Author Member

Good point, this probably would have broken on older Node.

@Kegsay

This comment has been minimized.

Copy link
Contributor

Kegsay commented Apr 7, 2017

Otherwise LGTM

dbkr added 2 commits Apr 7, 2017
It's a lodash template though, so use lodash
@Kegsay

This comment has been minimized.

Copy link
Contributor

Kegsay commented Apr 7, 2017

LGTM

@dbkr dbkr merged commit 42e317d into develop Apr 7, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.