-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
[DEPRECATION RFC ember-data] deprecate errors changing record state #384
[DEPRECATION RFC ember-data] deprecate errors changing record state #384
Conversation
Great suggestion! 🏅 How about adding these methods on the model itself, i.e. To me it would feel more consistent with the current API, for example accessing relationships are via ( |
## Summary | ||
|
||
Deprecates the `add` `remove` and `clear` methods of `DS.Errors` in favor of `addError` | ||
`removeError` and `clearErrors`. |
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.
This uses the singular whereas the design uses the plural.
@sandstrom i'd rather not scope creep into changing the API itself; the goal of this rfc is to follow-through on the (now quite old) deprecation and make it actionable for users. On the question of moving the API to the model, I'd prefer to avoid adding more API surface area to a class users already subclass a lot.
This is a good point about consistency although i think the safer way to consolidate this would be to move |
@hjdivad Alright, you have a point regarding scope creep and the downside of adding more methods to model. Another concern is that the method names Add a flag somewhere (suggestions below) and when that flag is enabled, we use the new API. That way we can keep the cleaner api Places we could add a flag
(I'd prefer (3), but either would work) [1] https://www.emberjs.com/blog/2015/06/18/ember-data-1-13-released.html#toc_upgrade-guide @hjdivad @runspired What's your thoughts? |
@sandstrom I do like the idea of a flag rather than moving the methods. But the downside is the flag is all or nothing: in a large codebase it can be important to be able to migrate deprecations incrementally. Adding a flag to the call site does allow for incremental migration and from my point of view is roughly equivalent to making a new method. I don't have a strong opinion between the two choices. As i see it, each has a slight edge over the other:
I could go either way (although again I'm not a fan of a global flag due to the impact on incremental conversion). |
I agree with David on being opposed to a global flag. I am additionally against the local flag and in favor of the changes here as proposed for three reasons. I list them here in order of importance: Assuming a local opt-in API like the following: add(member: string, message: string|string[], optIntoFuture: boolean)
Not only would users have to opt-in at each call site to eliminate the deprecation and ensure their code did not break, but once the deprecation completed they would need to receive a new deprecation telling them to drop the third argument.
Both because I dislike the confusion with the mechanics of true |
@runspired Good arguments!
That said, this is only the voice of one person and if you, as the RFC champion, disagree I think we should go ahead with this PR anyway. I've declared my suggestions above and they've been discussed, which is the important part — if none of them end up in the final version (i.e. discarded) that's totally fine! 😄 So even though I have some concerns (1 & 2) I'm still overall positive to this RFC! 🏅 |
Ideas, feel free to add to list or claim: - [ ] ```I've been getting a lot of questions about how tree-shaking is coming along. I would be willing to train anyone that wants to help on what's already done and what still needs to be done. Disclaimer: It's a lot of work! #emberjs #EmberFest``` https://twitter.com/kellyselden/status/1050717338595745792 - [ ] Include summary from pixelhandler (and cc him) #issue-triage team: https://discordapp.com/channels/480462759797063690/480523776845414412/500377829452546058 - [x] From jenweber on #support-ember-times: Another times topic: emberjs/ember.js#16910 I would summarize as, "ember.js has an addition to the test suite to help with API documentation! In the past, you might have noticed that a small number of methods had missing docs. When code gets moved around in ember.js, such as putting functions in their own modules, it's easy to make mistakes that impact the documentation parser. This test builds the docs and checks them for unintentional changes. Thanks <link to ed> for the test and to everyone who helped fix missing docs over the years." Todd Jordan did the majority of the fixes when things got dropped, I think. Hard to say for sure 🔏 @kennethlarsen - [x] emberjs/rfcs#384 (:lock: @jessica-jordan) - [x] emberjs/rfcs#386 (:lock: @jessica-jordan) - [ ] emberjs/rfcs#387 (:lock: @jessica-jordan) - [x] emberjs/rfcs#388 (:lock: @Alonski) - [ ] emberjs/rfcs#389 - [x] EmberConf brainstorming session date, if confirmed (:lock: @amyrlam) - [ ] Hacktoberfest roundup? - [ ] Check https://github.com/jessica-jordan/whats-new-in-emberland - [x] [ember-self-focused](https://engineering.linkedin.com/blog/2018/10/making-ember-applications--ui-transitions-screen-reader-friendly) (🔒 @chrisrng ) - [ ] ... Ideas we are waiting on: - [ ] EmberCamp talks, deep dive? http://embercamp.com/ and https://github.com/ember-chicago/ember-camp-2018-notes ... Maybe we wait for the talk videos to be published? Keep an eye on #ember-camp in Discord. not out yet - [ ] GraphQL with Ember? https://emberfest.eu/schedule/#rocky-neurock Or maybe we can reach out to them for a post-EmberFest writeup? See also the blog post: https://medium.com/kloeckner-i/ember-and-graphql-8aa15f7a2554
Moving this to final comment period. We'll aim to merge next week, barring any additional concerns. Thanks very much @sandstrom @lindyhopchris for taking the time to review and give feedback. 🎉 |
We landed a different approach to resolving this due to a desire to reconsider how we propagate errors through the system more holistically. |
Rendered