Feature/dapp prevent to load version directly from server #3025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The property of the root store that hold all information related to the version was already a "complex" object. There was a specific sub-set of mutations and getters just related to this property. This is a perfect indicator that this can be exported as its own independent store module. This change got triggered by the knowledge that these version information will become extended soon. Therefore this commit makes the refactoring to allow for better coding of the upcoming feature.
The service worker and the service worker assistant are exchanging messages. Therefore it is necessary to define a shared message en- and decoding. Messages include a fixed string (using an enumeration to match them) as identifier which type of message this is. Furthermore than can attach any kind of more payload to it. Unfortunately the last part has been implemented incorrectly in the past. This has not been detected yet because the payload was never evaluated so far. The only message that uses a payload so far is for installation errors by the service worker. The attached error object has been ignored by the assistant so far. Now it is necessary to have more messages with a payload where the assistant actually needs to access this data. The former implementation was based on a misunderstanding of the W3C Client interface to send messages. Instead of sending the payload with the message so the receiver can access it, the payload has been only transferred in ownership. But the receiver was not able to read this data. Therefore it is necessary to encode all the information into the `data` field of the message. So far this field was used for the fixed string to identify the message type. To solve the issue the data field has become an object with a required `messageIdentifier` property. This property acts the same as the whole field before and is used to match messages. The rest of the data object is then the optional payload that gets attached.
The Vuex store modules should be independent modules that don't care how the root store looks like. Unfortunately it was always necessary to reference the root state type in all modules so they know where they get mounted. This has become reworked by having the module define where the modules define where they will become mounted in the root store. Using this approach allows to easily and independently mounting modules where now the root store is responsible to ensure the typing is correct.
Codecov Report
@@ Coverage Diff @@
## master #3025 +/- ##
==========================================
+ Coverage 93.02% 93.04% +0.01%
==========================================
Files 208 212 +4
Lines 8447 8508 +61
Branches 1349 1362 +13
==========================================
+ Hits 7858 7916 +58
- Misses 482 484 +2
- Partials 107 108 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
andrevmatos
approved these changes
Dec 29, 2021
There was a problem hiding this comment.
Some nitpicks/suggestions, but LGTM overall. The test suite is quite nice as well, but since this uses a quite involved and not very well documented environment (service workers, caches et al), we need to do some good testing by hand as well before releasing. Thank you very much for taking the time to do this whole improvement.
Some Vuex store modules have a plugin that ensure that the modules state gets persisted to a storage backend. This ensures that the state survives a reload of the application. This feature is mostly implemented by a 3rd party library and was never tested. But it is an important feature of the dApp, especially for upcoming new store modules. Therefore we need to ensure that it works as intended. To enable testing, the store module plugins get now created by a function. This allows to specify an optional parameter to define the storage backend. Per default this is `window.localStorage` as always. The tests for the store module can now simply provide a mocked version of the storage backend. It is a clean separation of modules with clear interfaces. No need to mock global variables in an awkward manner. The test cases do not hide anything and can be read and understood easily.
|
I targeted all review comments with fixup commits. I'm confident about them, so I go ahead and resolve them. Then we have a state we can be subject of some final practical tests before we merge. |
weilbith
force-pushed
the
feature/dapp-prevent-to-load-version-directly-from-server
branch
from
897f35c to
1b18a76
Compare
Knowing which version of the dApp has been installed is an important piece of information. Having this data allows for future features to work with different versions and ensure strict manual updates. To do so, the service worker is sending a new message to the service worker assistant when it installed a new version and got activated. The message includes the installed version. The assistant will forward this data and put it into the version information store of the dApp. Having this data allows to compare the available version on the server with the actual locally installed version to determine if there is an update available. Before this was compared to the loaded version, which must not be correct all the time. The installed version information in the store is persisted to the local storage of the browser, as it gets only set when a new version gets installed. This property is the only one that gets stored from the version information store module. Everything else is and must be temporary.
The simple reason for this is that this component will be responsible for a wider set of just plain updates. It kind of already is. To reflect this better in the naming, the component got renamed to a more generic name while still maintaining its responsibility and relationship. This is a single commit to keep the history cleaner and better to understand.
Combining the now available information of the installed version and the active version allows to determine if the correct version got loaded. If this is not the case, the dApp gets blocked for the user to prevent him from using this wrong version (he did not confirm to update to). Further measurements will be added to resolve this situation.
The dApp gets blocked for the user when the wrong version got loaded. It is necessary to resolve this situation automatically. The approach to do so is to unregister all service workers and reload the tab. This solution has been derived from practical tests with force-reloads to bypass the service worker. Once the dApp got loaded, the service worker assistant gets triggered to verify if the correct version got loaded and if not he resolves it. To allow for proper testing of this feature, it is necessary to mock all parts of the global browser environment the service worker assistant is using. Such include for example the service worker container or the fetch function. In the past this has been tested with "complicated" global mocks in before and after test suite hooks. This isn't a too nice approach and should be avoided where possible. Therefore the assistant can be instantiated with an optional last parameter to define any sub-set of environment objects. Tests do now precisely define what they need individually and locally.
weilbith
force-pushed
the
feature/dapp-prevent-to-load-version-directly-from-server
branch
from
1b18a76 to
cab00a2
Compare
|
Sorry, for the linting mistake. Rooky mistake... |
|
We made a more production realistic test. And we discovered an issue. I made a mistake an forgot that service workers must be able to understand messages of the old application and send messages to the old application and new one understands them. This is a complex issue. |
weilbith
force-pushed
the
feature/dapp-prevent-to-load-version-directly-from-server
branch
from
9820221 to
e60a2d4
Compare
|
Because I haven't worked with schema versioning for a while I read this book tonight to recap a little. The used approach is explained in much detail within the related commit message. Please check it out. |
andrevmatos
reviewed
Dec 30, 2021
andrevmatos
reviewed
Dec 30, 2021
The service worker thread and the service worker assistant in the client thread exchange messages. Recently it was necessary to extend the message encoding to add some payload to them. Before that change, messages where pure Strings. The whole string has been used to identify which type of message this is to call the respective handler. Now the message type is encoded into an Object with possible additional payload. This was a breaking change in how messages are formatted. With this change the senders and receivers have been adapted to work with this new format. But what has been forgotten was that the service worker version and the client version can differ. In fact it is possible that a client of version X gets served by a service worker of version Y where Y > X. Each time a new version of both gets deployed to the public hosted server, the browser will load the new available script of the service worker. But the locally installed client (cached assets served by service worker) remains the old one. In result does the client send messages to the service worker it does not understand and vice versa. Despite the offline functionality this breaks all other features, including the update mechanism. To solve this problem it is necessary to establish a message formatting that is version agnostic. The topic of having different versions of the same event/message is typical and quite complex one. Especially in environments where different units of the communication run on different versions. The here chosen approach is to use a wrapper around the message and an internal mapping between the week schema (JSON) and the stronger internal typing schema. This approach requires to never rename any message property and only add new ones. Furthermore must the set of required properties be fixed and does not change with new versions. Only this way can an old receiver understand the message of a new sender. But also new receivers must be able to handle messages by old senders which is guaranteed due to the required fields. Having this methodology in place allows to have no strict versioning of the message scheme and allows for independent sender and receiver versions. Unfortunately there is already a version out there which acts on the old messages. The initial definition with the required fields etc are not known to them. To work with these versions, it is necessary add an exception to the whole messaging process, combining it with other approaches of message versioning. The service worker is never older than the client. This is a premise. To work with old clients before the new messaging format it must do two things. First it must be able to parse both message formats to act on messages from old and new clients. Second it must send messages in old and new format too. This means each message the service worker sends get send twice to each client, once in each format. Luckily this is only necessary for all message type which are supported until now. All future upcoming message types will not be handled by old clients anyway. This saves traffic in future and reduces the amount of necessary tests. The only thing the client has to do is to ignore messages in the old format. As the service workers sends these messages twice, it should not act on both of them. Old clients which are already released will in turn only act on the messages in the old format and ignore messages in the new format already (luckily). Having this mechanism once in place allows to establish the new enriched messaging format important for new features. And it is open for any new extensions of the messages, using the same format and being backwards compatible to old sender and receiver. Please note that the unit tests got adapted to don't use the message type enumeration and avoid to use any messaging helper where possible. This should ensure that the tests are very strict and do not get affected by code refactoring tools. This is necessary because old client/service worker versions will not get this refactoring and new versions would break compatibility.
weilbith
force-pushed
the
feature/dapp-prevent-to-load-version-directly-from-server
branch
from
e60a2d4 to
caf8565
Compare
The feature to verify if the correct version got loaded was so far only working for new versions. Loading a new version on top of an old one (i.e. <= `2.0.1`) will not be detected. But this is actually possible as a new version gets loaded that include function functionality to do so. The only issue so far was that the mechanism relies on the information that the client persists the information which version got loaded. Old version have not done this. But this information can be used. Should mean when there is no installed version number available, this means the installed version must be <= 2.0.1. If the active version is newer than that, it is wrong. This change required to adapt the update mechanism. So far was the installed version number deleted during an update as the check happens faster than the installation. As this would now lead to wrong implications, the installed version number never gets deleted. Rather a new flag of the store gets persisted that gets set during an update. When this flag is set, the check always passes as there can't be a wrong version loaded. As soon as the installation of the updated completed this flag gets reset again, allowing to detect wrong versions on the next load again.
|
Andre made the suggestion that we can also protect all versions before the new release. We planned the design and I'm implementing it. |
When an update is in progress, the new UI gets blocked for the user and a progress spinner was shown. This disappears after the page reloads. But here the actual installation actually happens. Having the new persisted information that an update is in progress this can be lengthened over the reload. The UI remains remains blocked until the service worker is done with the installation. The snackbar with the spinner is also active during this time. Furthermore this fixes the issue that after the latest changes the update is available message was shown while the update was still being installed.
|
Our production close tests worked very well. We even made some last UI improvements. Waiting for CI now. |
andrevmatos
approved these changes
Dec 30, 2021
weilbith
deleted the
feature/dapp-prevent-to-load-version-directly-from-server
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thank you for submitting this pull request :)
Fixes #3024
Short description
Checkout the issue and the commit messages for details.
Definition of Done
Steps to manually test the change (dApp)
yarn build)npx serve ./dist)Clear Site Data)package.json(e.g.3.0.0)yarn build)Shift+F5in Chrome)