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

Remove Proxy Symbol Workaround for Flexible Sync #5520

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

takameyer
Copy link
Contributor

@takameyer takameyer commented Mar 2, 2023

What, How & Why?

According to the workaround comments, the symbol injection for detecting proxy collections in flexible sync is no longer needed. I have removed the workaround and successfully tested it in the example application. This PR reverts the changes from #4541.

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 🔀 Executed flexible sync tests locally if modifying flexible sync
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated

@kraenhansen
Copy link
Member

If there's no harm in keeping it in, I'd rather that we don't touch master unless we really need to backport some bugfix. This feels like a simplification/ enhancement which does have some potential to break stuff if it doesn't work on some ancient version of an os / configuration.

@takameyer
Copy link
Contributor Author

takameyer commented Mar 2, 2023

@kraenhansen It's currently blocking me. Without this change, the jest tests won't allow me to console.log a collection. It's baffling, but I really don't want to dig deeper. After cherry-picking this change into my current work, things are working fine.

Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

Sorry for the fast approval, I had a deeper look at the proposed changes.
Are the deletion of lib/mutable-subscription-set.js intentional? With that we can't release a v11?

@takameyer
Copy link
Contributor Author

Are the deletion of lib/mutable-subscription-set.js intentional?

Yes, it was added only for this symbol hack. See https://github.com/realm/realm-js/pull/4541/files

@takameyer takameyer merged commit faf548e into main Apr 4, 2023
@takameyer takameyer deleted the andrew/remove-proxy-symbol branch April 4, 2023 09:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants