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

Embedded records support #3

Closed
wants to merge 19 commits into from

Conversation

flvrone
Copy link
Collaborator

@flvrone flvrone commented Oct 11, 2018

[ partly resolves #1 ]

Hey @janko-m,

there are no tests for #swap yet, I was trying to figure out how to properly test it, but then I gave up, and decided to leave it to you.

And you're going to change those methods anyway, to achieve proper "overriding" and "order independency" in plugins inclusion.

And thanks for all your help with this, I probably wouldn't make it alone :)

@janko
Copy link
Member

janko commented Oct 13, 2018

Had time to take a better look at this, and it looks great, very nice work! 👍

I will try to merge this in the next few days and release a new version, then eventually I will fix Shrine so that we can lose the order dependency.

@flvrone
Copy link
Collaborator Author

flvrone commented Oct 19, 2018

Hey @janko-m, I've found a bug with #swap method, it's related to those nested attributes here #4.

If there are few different embedded associations with files, for some strange reason, not all of them could be "uploaded" using my workaround for nested attributes.

Everything works fine if I switch to this commit LinkUpStudio@2faf5f6, where #swap is not modified. (And backgrounding breaks of course 🙂)

@flvrone
Copy link
Collaborator Author

flvrone commented Oct 23, 2018

Hey @janko-m,

I just realized that embedded records could be embedded into other embedded record, so embedding depth could be any, and this code currently supports only simplest scenario. So we should also cover those "recursive" embeds.

@flvrone
Copy link
Collaborator Author

flvrone commented Oct 23, 2018

OK, I've figured out what's the problem with #swap method.
You see, we have adapted it for backgrounding plugin, and when we do not use backgrounding - there will be no need in reloading, which is breaking everything.

I believe we have to check if backgrounding plugin is included and then use #swap method accordingly. (e.g. reload only when we deal with backgrounding).

@flvrone
Copy link
Collaborator Author

flvrone commented Oct 23, 2018

Added (I believe dirty) check for backgrounding, so now nested attributes are working properly for embedded models, if cascade_callbacks are enabled on association.

@janko
Copy link
Member

janko commented Oct 31, 2018

@FunkyloverOne Thanks for updating. Sorry, I'm still keeping this PR in mind and it will definitely get merged, I just wasn't able to find time yet.

@flvrone
Copy link
Collaborator Author

flvrone commented Oct 31, 2018

@janko-m no problem, it still needs "recursive" embeds support to be implemented, so it's not finished anyway. I don't really have time for it just yet too.

@janko
Copy link
Member

janko commented Mar 9, 2019

@FunkyloverOne I'm impressed with your pull request, and I realized I'm not able to find time to properly review it. So I added you as a collaborator and gave you rubygems.org access, and you're free to merge the PR and push a new gem version if you want.

@janko
Copy link
Member

janko commented Sep 15, 2019

Closing, as these changes won't be needed anymore in Shrine 3.0, see #1 (comment).

@janko janko closed this Sep 15, 2019
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.

Backgrounding for embedded models
2 participants