-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Incompatibility with ember-data 3.13.0 #338
Comments
I appear to have the same issue on 3.12 |
with stack trace this time:
|
emberjs/data#6247, which implements RFC 403: Ember Data | Identifiers, has changed the interface of |
@jelhan thanks for investigating. I'll try to get a fix in when I get some time but if someone makes a PR before then I'll merge it quickly. |
edit: Turns out, there was a sub dependency of Ember-data that was upgraded to 3.13 on my end, so I can confirm its not happening on 3.12. |
Fixed the changed interface on |
@jakesjews You mention in #339 (comment) that Model Fragment will need to be refactored to use
Not knowing the internals here, do you have a sense of how much work that is? |
I have a feeling that it's almost a complete rewrite. A spike of the conversion was attempted here but I don't think it got too far. |
Just when i started being ok with ED and its quirks.... it rips the carpet out from under me again... 😿 |
I can understand that you are pissed of by breaking your production build but I don't think it's Ember Data's fault that this addon heavily relies on private APIs. Public APIs were discussed and defined as part of the RFC process with this addon in mind long time ago. Ember Data has exported some private APIs only to not break this addon. At some point of time this gets to costy and the private API has to be removed. I think it was long enough for all consumers of this addon (including myself) to start refactoring it based on public APIs.
I don't want to offend you but it's not the fault of Ember Data or the people working on it if your tests and QA aren't catching this bug before shipped to production... |
Q: Is there any effort underway to support ED 3.13? Or are we just sitting here.. :-) |
To be honest I don't really have the time to rewrite it. I took over maintenance a few years ago with the intention of doing simple ember upgrades and merging bugfixes but I don't really have too deep of an understanding of how everything works. |
I've tried working with recordData in the past, the main problem i have is that its not well documented and the documentation there does not reflect real behavior. I kinda gave up on it. Maybe its gotten more stable since its first release. |
It would be sad to see this addon go dead because of this, it is a wonderful addon, which adds a very welcome addition to the Ember Data platform in a whole. I'd be willing to commit some time as well, but I have no clear view of how RecordData works, let alone Ember Data internals. I can help debugging or test running perhaps. |
I totally agree with @pjcarly My project depends pretty heavily on this addon... Willing to do some work. Anyone can outline what the problem is and where we would need to start to get repairs going? An overview of what RecordData is and how they compare to the current workings would probably help a bit too... |
Ping @runspired @kategengler I see you coauthored https://github.com/emberjs/rfcs/blob/master/text/0293-record-data.md#addon-usage and you mention a path forward and even some experimentation already done if I read it correctly. Is any of you able to provide some steering directions? Thanks! |
@basz I'd also be keen to help out here too as we're heavily dependent on Model Fragments, although at this stage it won't be until the new year until I can give time |
@jakesjews That's fine. As long as someone is available to review/merge the community seems willing to get this going. @ALL To start I've updated the tests to work again, so we have something to work off |
Happy to provide a review, @igorT had tried to spike this before, I suspect we could again. |
We're also pretty heavily dependent on this, and will be stuck on 3.12 until this get's fixed, so we're happy to help out. Has anybody made any progress on this? If not, we may try to spike this. |
I'm working (slowly) on a fix for this. Have tests at least running and failing for proper reasons on 3.12. I'll update here as I have more information :-D. |
We reviewed the usage of Ember Data Model Fragments and noticed that it's in most cases only used to have a schema for complex data types like POJOs and arrays of POJOs. We noticed that we could achieve the same with native classes and Ember Data transforms. We decided to drop Ember Data Model Fragments. Please note that this approach is only providing a subset of features that Ember Data Model Fragments had. It's only a schema + serializer. If you need dirty tracking of Ember Data model to work you must follow an immutable pattern for that objects / collection of objects. Assigning a POJO to the attribute and have it automatically converted to an instance of that class would also not work. These limitations simplify the code and fit better to Octane pattern in my opinion. But you may have another feeling. I would recommend to play around with that approach before making a decision. I can try to write a more detailed blog post about this approach. But not sure when I will have the time to do so. |
Update - I have a branch (richgt/ember-data.model-fragments -> richgt/model-data-ember-3.13 that has 5 failing tests remaining. I'm a little stumped on the remaining tests, if anybody has any ideas, I'd love some help here... |
Noticed that my last post might be confusing. It is unclear what "we" refers to. It refers to the company for which I'm currently working. |
May I call attention to @richgt failing tests? I have taken a look but really have no clue where to look, totlly not an expert here. Three failing tests seems to be related to (de)serialization.
These seem to be related to "Cannot clone an EmberObject that does not implement Copyable"
Seems we are close though... |
@jelhan Do you have time to you elaborate on #338 (comment) a bit more? I feel that might help some users to make an informed decision to abandon this addon or not. |
If you want to compare that feature branch against latest development: master...richgt:richgt/model-data-ember-3.13 TravisCI seems to be broken which makes it difficult to quickly review broken tests: https://travis-ci.com/github/richgt/ember-data.model-fragments/branches @richgt Could you maybe create a pull request? Would make tracking progress much easier. The failing tests posted by @basz seems to be similar to the ones that were still failing on #339:
These two were showstoppers. You can find details in the merge request. Maybe I missed something on the investigation or maybe Ember Data changed but at the point of time I was trying to do the upgrade these two failing tests were indications of a fundamental issue in the architecture. @richgt If you haven't done yet, I would strongly recommend to have a look in my pull request #339
Will try to write a blog post in the next weeks. But have a lot of work to do. Not sure if I actual can do so and can not commit to a deadline. |
Created #360 |
@basz I have also stopped using e-d-m-f. The addon broke a couple of times in the last few years after e-d updates/refactorings. If all you need, like in my case, is a simple object with maybe a few attributes parsed into a custom data type the burden of managing library breakage when upgrading is not justified IMHO. You are better off writing a custom transformer, which e-d has always allowed you to. I think this addon is great, but for simple use cases like mine I think it is just not worth it. |
Are there other solutions for those that need dirtying? I have probably 20+ Fragments. |
ember-changeset provides a nice way to track changes. Not integrated with Ember Data but that's more a feature than a limitation in my opinion. Might be require quiet some refactoring if the app is currently relying on Ember Data and Ember Data Model Fragments for change tracking. |
FYI, I hope to have a release out that works with 3.13+ sometime next week. |
@jelhan good suggestion. Ultimately I want to make this an immutable object, but it'll be a bit before we can get there with time constraints. |
For everyone watching this issue there's a 5.0-beta release with the |
So far it's working as expected, so good work! Thanks to everyone who made this possible... |
Working on my end so far |
This error appears as soon as you upgrade from e-d 3.12.x to e-d 3.13.0
The text was updated successfully, but these errors were encountered: