-
Notifications
You must be signed in to change notification settings - Fork 15
fix(android): separate local storage between WebView processes #106
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
Open
ItsChaceD
wants to merge
10
commits into
main
Choose a base branch
from
fix/RMET-4918/isolate-local-storage
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
d51bbd8
fix(android): separate local storage between WebView processes
ItsChaceD fa5d3e8
add isIsolated config value
ItsChaceD 380fa48
Merge branch 'main' into fix/RMET-4918/isolate-local-storage
ItsChaceD 420d53d
merge conflicts and update aar version
ItsChaceD 0c11349
prepare script
ItsChaceD 5f01d43
fix lib file location
ItsChaceD 6ee9184
fix merge conflicts in changelog and package
ItsChaceD 69bb177
remove activeRouter = null
ItsChaceD 92f2e1f
fix docusaurus syntax
ItsChaceD df47753
use published lib version not aar
ItsChaceD File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| import React, { useState, useEffect } from 'react'; | ||
| import { IonContent, IonHeader, IonPage, IonTitle, IonToolbar, IonButton, IonInput, IonItem, IonLabel, IonList, IonButtons, IonBackButton } from '@ionic/react'; | ||
|
|
||
| const LocalStorageTest: React.FC = () => { | ||
| const [value, setValue] = useState(''); | ||
| const [storedValue, setStoredValue] = useState<string | null>(null); | ||
|
|
||
| const refreshValue = () => { | ||
| const val = localStorage.getItem('inappbrowser_test_key'); | ||
| setStoredValue(val); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| refreshValue(); | ||
| }, []); | ||
|
|
||
| const saveValue = () => { | ||
| localStorage.setItem('inappbrowser_test_key', value); | ||
| refreshValue(); | ||
| }; | ||
|
|
||
| const clearValue = () => { | ||
| localStorage.removeItem('inappbrowser_test_key'); | ||
| refreshValue(); | ||
| }; | ||
|
|
||
| return ( | ||
| <IonPage> | ||
| <IonHeader> | ||
| <IonToolbar> | ||
| <IonButtons slot="start"> | ||
| <IonBackButton defaultHref="/home" /> | ||
| </IonButtons> | ||
| <IonTitle>LocalStorage Test</IonTitle> | ||
| </IonToolbar> | ||
| </IonHeader> | ||
| <IonContent className="ion-padding"> | ||
| <IonList> | ||
| <IonItem> | ||
| <IonLabel position="stacked">Test Value</IonLabel> | ||
| <IonInput value={value} onIonChange={(e: any) => setValue(e.detail.value!)} placeholder="Enter value to store" /> | ||
| </IonItem> | ||
| </IonList> | ||
| <div style={{ marginTop: '20px' }}> | ||
| <IonButton expand="block" onClick={saveValue}>Save to LocalStorage</IonButton> | ||
| <IonButton expand="block" color="light" onClick={refreshValue}>Refresh from LocalStorage</IonButton> | ||
| <IonButton expand="block" color="danger" onClick={clearValue}>Clear LocalStorage</IonButton> | ||
| </div> | ||
|
|
||
| <div style={{ marginTop: '40px' }}> | ||
| <h3>Stored Value:</h3> | ||
| <p style={{ fontSize: '24px', fontWeight: 'bold' }}>{storedValue || '(none)'}</p> | ||
| </div> | ||
|
|
||
| <div style={{ marginTop: '20px', padding: '10px', backgroundColor: '#f0f0f0', borderRadius: '8px' }}> | ||
| <p><strong>Instructions:</strong></p> | ||
| <ol> | ||
| <li>Set a value here in the main app.</li> | ||
| <li>Go back and open this same page using "Open Site (InAppBrowser)".</li> | ||
| <li>Use the <strong>Enable Storage Isolation</strong> toggle to test different modes. | ||
| <ul> | ||
| <li><strong>On (Default)</strong>: Value should be "(none)".</li> | ||
| <li><strong>Off</strong>: Value should be shared.</li> | ||
| </ul> | ||
| </li> | ||
| </ol> | ||
| </div> | ||
| </IonContent> | ||
| </IonPage> | ||
| ); | ||
| }; | ||
|
|
||
| export default LocalStorageTest; |
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
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.
Note about marking this as a breaking change / major version in the Capacitor plugin: Either the squash-commit needs to have a
BREAKING CHANGE: <describe breaking change>in the commit footer so thatsemantic-releasedoes the major version bump (should try adry-runlocally on main just to check), or we do a manual release.Plus (not applicable to this PR specifically) we should create a 3.x branch before this PR is merged, but as for releasing patches in 3.x, I'm not sure how to approach this (2.x is going to latest-7, would 3.x go to latest-8, and where would 4.x go to if we do Capacitor-9 related breaking changes in a 5.x?), because up until now we've had one major version per Capacitor major version, think this will be the first plugin to break this rule. Maybe this needs to be brought up to the team internally, but mentioning it here because I think we haven't had this dicussion.
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 added the exclamation mark just in case, but will also add it to the footer of the squashed commit.
As far as the versioning, you bring up a valid point. I think in this case, we should just create a
3.xbranch and merge this there along with any other Capacitor 9 upgrades we will do in the upcoming weeks. This "bug" wasn't reported by any customer / user except internally so I don't believe it is very urgent.For future breaking changes, it will probably be a case-by-case basis, but if we target only 1 release of a major version per year for Capacitor.. then we will probably eventually run into scenarios where we will have to break the rule, especially if its an urgent bug fix or something
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.
Exclamation mark won't work, because semantic-release in this repo is following Angular Commit convention, not conventional commits, where there's no "!"; I'd say probably it's best not to have one.
Not sure I follow. If we want to bring this fix to OutSystems, we need to update the Capacitor plugin and use it on ODC.
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.
Sorry I meant to advocate for making the
3.xbranch, but only merge this change tomain. That way it is only included with the next major release which will likely be with the other Capacitor 9 upgrades. I don't think this needs to make it into the current version since it's not as urgent and might be weird for us to be making 2 major releases for a single Capacitor version unless it's necessary, especially since there isn't a precedent and then we'd be releasing another major release right after.I will bring it up to the team to see their thoughts because I think it is an important discussion
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.
Per internal discussions we're fine with raising the major version now, just keep in mind the initial comment I made that I think is still relevant regarding how to do the version bump + (and I don't think we discussed this with the team, or maybe we did and I forgot) should we keep maintaining 3.x after releasing 4.x?
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.
Yes I will add the
BREAKING CHANGE:in the commit footer.As far as maintenance goes for
3.x, I don't think it's absolutely necessary if we don't have the bandwidth as users can upgrade to4.xand keep the same behavior by simply disablingisIsolated. However, it also wouldn't hurt to just cherry pick all of the changes to3.xto be safe. In the past we have always just supported the current version and one previous major version, so if we follow that pattern then maybe we maintain it until5.xcomes out.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.
The one doubt I have is what npm tag would
3.xgo to? because we have been using latest-Y where Y is the capacitor framework major version that the plugin version is compatible with. Since there'd be two (3.xand4.x), unsure where that would goBut this isn't something that we need right now I guess. As long as we have
3.xbranch, we can set the rest up any other time if we find it's better to have those two versions, along with2.xas well (for Cap 7 until later this year).