-
-
Notifications
You must be signed in to change notification settings - Fork 596
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
feat: Add two-way binding of Parse object properties with Parse.Object.bind.key = value
#1484
base: alpha
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request!
|
For those unfamiliar with VueJS, you can bind object properties and they are automatically updated by their respective component.
Screen.Recording.2022-05-24.at.9.15.39.pm.mov |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## alpha #1484 +/- ##
==========================================
- Coverage 99.90% 99.88% -0.02%
==========================================
Files 61 62 +1
Lines 6104 6176 +72
Branches 1482 1494 +12
==========================================
+ Hits 6098 6169 +71
- Misses 6 7 +1
☔ View full report in Codecov by Sentry. |
This seems like a convenient feature, but I'm not sure this would work. How would you prevent collisions between a native JS object property and a Parse.Object property of the same name? Related to this is parse-community/parse-server#7130 where the discussion goes into the contrary direction to allow any field name, because a developer shouldn't have to worry about or be limited by collisions with Parse Server internal or JS native field names.
We abandoned the concept of "experimental" features. If a feature is merged, it has the alpha / beta pre-release periods to mature, effectively 2 months minimum. If that's not enough because the feature is still too unstable or we haven't fully explored potential side effects, it won't be merged into the codebase for general stability and security reasons. |
I think convenience and ease of use are important - especially for newer developers. There is a learning curve with Parse and this feature makes Parse objects behave intuitively instead of the convoluted
Can you give an example? I'm sure it would be easy enough to check if Well, if it's not suitable for default usage, it would be good to have it as an option so Parse becomes a lot more usable with Vue. The only way to be assured that dot notation works for every single use case, would be to re-write every test with the additional dot notation syntax. I think for a new feature it's expected that it matures over time. |
As I said, we don't have the concept of experimental codebase changes at this time, even if part of the feature is a new Parse Server option. The feature would need to already have a certain level of maturity to enter the alpha branch, to a degree where it's reasonable to believe that it will become production ready within 1 month, before it moves to beta. |
Well any initial release of software is expected to become more stable over time. All I’m saying is that there may be errors thrown specifically relating to the dot notation that might need refining / future PRs, which is expected of new features in an open source environment. I’m still not convinced there’s enough reason not to consider this optional feature, especially as it dramatically increases usability with VueJS. If it’s a feature you wouldn’t use in your Parse environment, you don’t have to, but I don’t see why other developers should be restricted. |
Sure, if there are no open concerns this PR will be merged like any other. What I'm saying is that at this time we don't merge a PR if we do not believe in best conscience that it is production ready. I'm not saying this PR is or is not production ready, that's a different question. I've opened parse-community/parse-server#8016 to hopefully give you more insight into the challenges of "experimental" features. |
Ahh, ok. Sorry we got a bit carried away. I'll continue to test this feature on my servers / sdks and let you know when I feel it's ready. |
Always good to have a healthy debate. I think if this is technically feasible, it would be a nice feature to have, not just for Vue developers. I will pin the issue to hopefully give it more exposure and have more eyes on it. Just thinking, if you enabled the feature in a branch and did a regex replace for all tests from The reason I think this is a PR we have to look at very carefully is because it removes the containment of |
That's a good idea, thanks.
Well I think this could be a documented side-effect of this feature. The same functionally happens in the Swift SDK without any troubles. Proxy objects merely intercept gets and sets too, no data is stored on the respective keys. For example, console.log on |
What do you mean by "side-effect"? How should a developer interpret this? Are there any circumstances under which this change could break something? |
Maybe the changelog entry could mention some keywords like "vue"? How about:
|
Let me have a quick look to see if applies to Svelte too |
mov.movFeature is fully compatible with Svelte as well, with the only caveat being that to trigger reactivity on save, Svelte demo: https://github.com/dblythy/Parse-Svelte-Binding And for good measure, AngularJS demo: https://github.com/dblythy/Parse-Angular-Binding |
Can the docs all be packed into the |
I have added some JSDocs, but it should be part of the JS Guide as well |
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.
Looks good, is this ready for merge?
Did you want to modify the docs at all? I didn't make too much effort with it as I figured you were likely to amend |
Maybe this is something we could incorporate into the JSDoc:
Could you formulate that in more generic terms? Should we add code example snippets too for 2 way binding with vue, svelte and angular since you mention they use a different syntax? |
Perhaps we can add this detail in the expanded Javascript Guide, rather than the JS Docs? |
I'd only add to the docs what cannot go into JSDocs for some reason. I think keeping docs as close to the code as possible makes most sense, because:
A separate documentation may be more for in-depth explanations where necessary or more tutorial-style introductions. I personally believe that's what we should aim for, but we haven't introduced a definitive policy on that yet. Apple's own SDK docs are a good example. So depending on what you have in mind and how extensive of an introduction you want to write, you can decide for one or the other. However, we definitely want to avoid duplicate docs in both places. |
@@ -148,6 +150,17 @@ class ParseObject { | |||
_objCount: number; | |||
className: string; | |||
|
|||
/** | |||
* Bind, used for two way directonal binding using |
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.
Needs completion
Working on improving Array support |
Hello, do you have any news about this feature being released? |
It seems that only the docs could benefit from some of the comments mentioned here, see #1484 (comment); then this could be merged. |
Great! I'm using the proxyHandler hack, but it seems to be causing side effects. I'm still new to Parse and Vue, and this feature will greatly ease the learning process. |
New Pull Request Checklist
Issue Description
When using VueJS, it becomes difficult to manage Parse Objects via their native
.get
and.set
syntax. Also, for newer developers who are use to standard JSON dot notation, the custom Parse Object getters and setters can be convoluted.Related issue: #1313
Approach
Adds
object.bind
option that allows for getting and setting of Parse Object properties via dot notation.TODOs before merging