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

Implement Parent tracking #35

Merged
merged 5 commits into from Sep 19, 2019

Conversation

@blaze182
Copy link
Contributor

@blaze182 blaze182 commented Sep 6, 2019

G'day, trying to resolve #31 :)

This is still work in progress, needs some cleaning up, but parent is finally there, at least locally :)

Like StoreModel::Types::*, ActiveModel::Attribute initializer unfortunately doesn't have access to ActiveRecord/ActiveModel context as well, so I tried to introduce the patches to provide it during AttributeSet initialization and modification.

Hope the overall direction makes sense, open to any suggestions

@blaze182 blaze182 force-pushed the feature/track_parent branch from 93e2c12 to 9998c33 Sep 6, 2019
Copy link
Owner

@DmitryTsepelev DmitryTsepelev left a comment

Hi @blaze182, that's a good start!

I've made a couple of comments here and there, but it feels like there should be a place in the Rails codebase where we have an access to both AR model and the attribute. A quick search brought me here.

This is the patch that made your spec pass:

ActiveRecord::Base.prepend(Module.new do
  def _read_attribute(*)
    super.tap do |attr|
      attr.parent = self if attr.is_a?(StoreModel::Model)
    end
  end
end)

I'm definitely not sure that it's a single place we have to patch (e.g. it does not handle the case when we assign the attribute value to the AR model etc.), but the overall approach should help avoiding the context and passing it here and there 🙂What do you think?

lib/active_model/ext/attribute_set.rb Outdated Show resolved Hide resolved
lib/active_model/ext/attribute.rb Outdated Show resolved Hide resolved
lib/active_model/ext/attribute.rb Outdated Show resolved Hide resolved
@blaze182
Copy link
Contributor Author

@blaze182 blaze182 commented Sep 8, 2019

Thanks for the feedback!
Regarding the override strategy, I was also thinking about just patching AR’s read_attribute, as it appeared to be the first place having both contexts in the caller chain. But yeah, as you said we need to cover at least three more cases, (1) attribute assignment, (2) same read/(3)write cases for plain ActiveModel instances (i.e. we can nest StoreModels). And same set for Array type, which in case of read/write override should have another condition I guess. Gotta add these to rspec suite btw :)

The motivation behind preserving parent reference and passing it top-down was attempt to eliminate possible edge-cases (i.e. reassignments, ActiveModel::Dirty tracking, attributes restoring and reloads etc.) by having the StoreModel initiated with parent straight away. As a bonus, we are leaving parent assignment logic encapsulated in Types :)
Anyway I can try looking into read/write override approach as well tomorrow.

Sent from phone, so pardon any typos

@blaze182 blaze182 force-pushed the feature/track_parent branch from 866578a to 8c47a55 Sep 8, 2019
@blaze182
Copy link
Contributor Author

@blaze182 blaze182 commented Sep 9, 2019

After digging deeper I found out that context approach isn't stable enough for attribute assignment case, so decided to proceed with read/write approach as you initially suggested. Looks like it's working now for AR objects for both json and array types :)
However still needs implementation for nested ActiveModels

@DmitryTsepelev
Copy link
Owner

@DmitryTsepelev DmitryTsepelev commented Sep 9, 2019

Wow, that looks short! Great progress!

@blaze182
Copy link
Contributor Author

@blaze182 blaze182 commented Sep 15, 2019

Hi Dmitry, hope you're doing well!
Sorry was busy during the week, but finally got my proposed solution more less finalized during this weekend, so keen to hear your feedback :)

The only limitation I see now is direct StoreModel's Array mutation, it won't update a parent until one of the accessor methods gets called. As far as I can see it's achievable if we could introduce StoreModel::Collection instead of Array, but I guess it should be treated as another feature.

@blaze182 blaze182 marked this pull request as ready for review Sep 15, 2019
@DmitryTsepelev
Copy link
Owner

@DmitryTsepelev DmitryTsepelev commented Sep 17, 2019

That looks awesome, thanks for the great work! I'll play with it a little bit on Thursday and come back with a feedback (if I bump into something). In the meantime, could you please add yourself into the changelog?

@blaze182 blaze182 force-pushed the feature/track_parent branch from e02c1e8 to 174422e Sep 18, 2019
@blaze182 blaze182 force-pushed the feature/track_parent branch from 174422e to 5cd4c8b Sep 18, 2019
@blaze182
Copy link
Contributor Author

@blaze182 blaze182 commented Sep 18, 2019

Thank you, let me know how it goes :)
Added an entry for master

Copy link
Owner

@DmitryTsepelev DmitryTsepelev left a comment

@blaze182 I've played with it and it looks amazing! I have only one question before merging it in and creating a follow-up issue for handling collections 🙂


private

def attribute(*)
Copy link
Owner

@DmitryTsepelev DmitryTsepelev Sep 19, 2019

Choose a reason for hiding this comment

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

Do you know the case when we need this patch? I've tried to remove it and all specs are passing, looks like this scenario is covered by #_read_attribute method

Copy link
Contributor Author

@blaze182 blaze182 Sep 19, 2019

Choose a reason for hiding this comment

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

Good catch! We had a preemptive parent assignment on attribute write in specs :)
I updated scenarios, so now couple specs should fail accordingly

@DmitryTsepelev
Copy link
Owner

@DmitryTsepelev DmitryTsepelev commented Sep 19, 2019

Merging, thanks again! 🎉

@DmitryTsepelev DmitryTsepelev merged commit 010c597 into DmitryTsepelev:master Sep 19, 2019
1 of 2 checks passed
@blaze182 blaze182 deleted the feature/track_parent branch Sep 19, 2019
@DmitryTsepelev
Copy link
Owner

@DmitryTsepelev DmitryTsepelev commented Nov 6, 2019

Hi @blaze182! Could you please send me your email and mailing address (we plan to send some goodies to CultOfMartians participants) to dmitrytsepelev@evilmartians.com?

@blaze182
Copy link
Contributor Author

@blaze182 blaze182 commented Nov 6, 2019

Woohoo, thanks @DmitryTsepelev! Just dropped you a line

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

Successfully merging this pull request may close these issues.

2 participants