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

fix: proxy localStorage and sessionStorage (closes #293) #548

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

n0th1ng-else
Copy link
Contributor

@n0th1ng-else n0th1ng-else commented Jan 31, 2024

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

In the current implementation, we cache both storages (localStorage and sessionStorage) at the moment of worker creation. Although this could be considered as performance optimization, it leads to the implementation inconsistency:

  • Storage data leaks into 3rd party script
  • Storages easily go out of sync as there is no proper synchronization with main window setup.
  • One can not override the storage access easily.

For the latter point, this means we can not take on the methods like localStorage.getItem() in the Partytown proxy methods (specifically in apply). Hence there is no way to shield the storage data from 3rd party scripts.

In this commit we fully rely on the storage values the main window sends into the worker. As the result, we can always receive relevant value and also can override the storage api with apply and get hooks

Use cases and why

  • Block accessing the localStorage:
    <script>
      partytown = {
        apply: opts => {
          if ('localStorage.getItem' === opts.name) {
			return null; // Block localStorage access
		  }
          return opts.continue;
        }
      };
    </script>

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality
Before
image
After
image

Copy link

vercel bot commented Jan 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
partytown ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 1, 2024 0:33am

@n0th1ng-else n0th1ng-else changed the title Proxy localStorage and sessionStorage (closes #293) fox: proxy localStorage and sessionStorage (closes #293) Jan 31, 2024
@n0th1ng-else n0th1ng-else changed the title fox: proxy localStorage and sessionStorage (closes #293) fix: proxy localStorage and sessionStorage (closes #293) Jan 31, 2024
@n0th1ng-else n0th1ng-else marked this pull request as ready for review January 31, 2024 20:06
@gioboa gioboa added waiting for core Waiting for one of the core team members to review and reply PR waiting for review This PR is waiting for review and approval before merge labels Jan 31, 2024
@gioboa
Copy link
Member

gioboa commented Jan 31, 2024

Can you commit your tests for this too pls?

@n0th1ng-else
Copy link
Contributor Author

I was also wondering if I could add the test for this. could you please advise me on how to do that?

do we have an example of one e2e test suite but with a different config? like, I see tests/platform/storage/index.html and ideally I think I would put more e2e tests in this folder, but then I am a bit lost how to do that

@gioboa
Copy link
Member

gioboa commented Jan 31, 2024

you can add another html file and use the goto method like this one

@n0th1ng-else
Copy link
Contributor Author

ah sure, its a playwright script in the end of the day 🤦🏿 I will add more tests and let you know 👍🏿

In the current implementation, we cache both storages (localStorage and sessionStorage) at the moment of
worker creation. Although this could be considered as performance optimization, it leads to the
implementation inconsistency:
- Storages data leaks into 3rd party script
- Storages easily go out of sync as there is no proper synchronization with main window setup.
- One can not override the storage access easily.

For the latter point, this means we can not take on the methods like `localStorage.getItem()` in the
Partytown proxy methods (specifically in `apply`). Hence there is no way to shield the storage data
from 3rd party scripts.

In this commit we fully rely on the storage values the main window sends into the worker. As the result,
we can always receive relevant value and also can override the storage api with `apply` and `get` hooks
@n0th1ng-else
Copy link
Contributor Author

I have added the tests now 👍🏿

@n0th1ng-else
Copy link
Contributor Author

@gioboa could you please look into it again?

Copy link
Member

@gioboa gioboa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @n0th1ng-else it looks great

@gioboa gioboa merged commit 66db16d into QwikDev:main Feb 14, 2024
14 checks passed
@n0th1ng-else n0th1ng-else deleted the storage-fix branch February 14, 2024 22:25
@n0th1ng-else
Copy link
Contributor Author

thank you Giorgio! Looking forward to contribute more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR waiting for review This PR is waiting for review and approval before merge waiting for core Waiting for one of the core team members to review and reply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants