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

Fixed persist crash when the stored value is undefined #4091

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

wahibimoh
Copy link
Contributor

Fixed persist crash when the stored value is undefined. This bug crashes Alpine

Fixed persist crash when the stored value is undefined.
This bug crashes Alpine
@calebporzio
Copy link
Collaborator

What do you think about instead of adding try/catch, we just get the value, check for undefined and if so, return that, otherwise return JSON.parse()?

This way it works as expected and doesn't have a console.warn in there (which I try to avoid as they take up an abnormal amount of size in the minified bundle)

@ekwoka
Copy link
Contributor

ekwoka commented Mar 14, 2024

That was my thought as well.

Whenever we check if the item even exists int he storage, we also check if it's undefined.

The one thing I think might need to be determined is the expected behavior of setting a persisted value to undefined. Should it be treated as non existent and reinitialize, or be treated as the value undefined.

I would think setting to null should be used for explicit, and undefined used to resetting.

@calebporzio
Copy link
Collaborator

@ekwoka, I'm not sure I understand. I updated the PR and merged. Please re-submit with changes if you think there's a better way. Thanks!

@calebporzio calebporzio merged commit c38a45f into alpinejs:main Mar 18, 2024
1 check passed
@wahibimoh
Copy link
Contributor Author

thanks for the refactor and merge, I am just thinking is undefined the only value that could cause an error JSON.parse? If yes, then it the refactor is perfect, if there are other values, then we may have to use try/catch or check for these values as well.

So far, I only faced this crash with undefined value in production.

@ekwoka
Copy link
Contributor

ekwoka commented Mar 19, 2024

Anything else would have had to be malformed in the first place...which would most likely only be possible if someone directly modified the storage. It wouldn't happen from within persist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants