-
Notifications
You must be signed in to change notification settings - Fork 139
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Signed-off-by: Christopher Rogers <[email protected]>
- Loading branch information
1 parent
ec5ba94
commit f70974e
Showing
4 changed files
with
10 additions
and
3 deletions.
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,4 +3,4 @@ | |
* Current package/component version. | ||
*/ | ||
|
||
module.exports = '4.9.0'; | ||
module.exports = '4.9.1'; |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
f70974e
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.
@chrissrogers hey Christopher , it seems like v4.9.1 just went to the CDN, right? I don't see a CDN date on v4.9.0 release though, maybe it wasn't pushed, and it was only included now?
As a FYI: This new release broke our subscription flow under one of our mobile apps through a WebView. Apparently it forced a requirement on/usage of
window.localStorage
to load the hosted fields, which wasn't available under that environment, I believe part of the changes included in #459. We're looking into pushing a hotfix to make sure that's available for the WebView. I understand it's extremely difficult to detect, but I wanted to give you a heads up on that, especially because I wasn't sure if v4.9.0 (which included that PR) was released to the CDN earlier.Maybe we could find a way to test these versions before they go to the CDN? If you have any suggestions on how we could do that, let me know. Thanks!
f70974e
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.
@carlosantoniodasilva thanks very much for reporting about that. Yes, we're now utilizing localStorage in v4.9.0+, so you're right on the money.
You're also correct in thinking that v4.9.0 was not released to the CDN, instead we went straight to v4.9.1 this time around. It's not common for us to do that, but since v4.9.1 was created in the testing and deployment interim, we went ahead with the newest version.
Really valuable insight about WebView storage availability. These were obviously not featured in our testing scenarios, and did not raise flags since storage is so widely available in our target platforms.
I've made a note to investigate this, and to ensure WebViews are featured more prominently in our future tests.
I assume your fix is to include a storage polyfill? If so, I estimate that will work well.
f70974e
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.
Reading into Android's WebView, it seems storage must be enabled when generating the WebView -- perhaps it is best for us to make a note of this necessity in documentation.
f70974e
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.
@chrissrogers right yes, we're enabling that option in the Android WebView context as a permanent solution, and looking into the polyfill in the meantime. 👍
I think it'd be nice to document the requirement on session/local storage going forward somewhere, even if only in the release notes. I had to do some diving into the latest PRs/changes to find that one.
It'd also be really great if you could somehow perform tests with the js in a similar WebView context too before pushing a release. I don't know how feasible that would be and/or how your testing normally works, but from our perspective it would be extremely helpful I think, could catch something like this. To be honest, it's the second time this particular subscription flow broke with a new version. :)
Thanks!
f70974e
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.
It's a good idea to include notes on new native APIs being utilized in new versions. We will only ever rely on widely-available APIs, but there are certainly edge cases like this that we would like to make merchants aware of ahead of time. I think release notes are a great location for those announcements.
We'll be looking into adding WebViews to our CI operations. There would be some config discrepancy of course between our implementation and others, but it's a start.
Thanks again for the notes!
f70974e
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.
Hello there!
Just want to add to this thread another scenario where the
localStorage
breaks.When I enable Chrome's "Block third-party cookies" option, I get an "Uncaught DOMException: Failed to read the 'localStorage' property from 'Window': Access is denied for this document." error.
We've been getting some reports of customer having issues because they have this feature turned on. It seems that this third-party cookie blocking is getting very popular, might have something to do with Firefox and Safari's updates.
Not me, I love 🍪 regardless of where they're coming from.
f70974e
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.
Mmm good note on that @guzart -- I think that warrants some additional safety checks for availability of storage. I've added that to the work queue for v4.9.2
f70974e
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.
Thank you @chrissrogers !
f70974e
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.
Just an update from our side -- #478 should address these cases of storage availability
f70974e
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.
Thanks @chrissrogers! Happy to see a new v4.9.2 release was just created containing that change. Do you have any idea when it will be pushed to the CDN and become generally available? Thanks.
f70974e
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'm expecting mid next week
f70974e
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.
Just wanted to let you know that we've deployed v4.9.2 to our CDN today -- please do let me know how it works for you!