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

Payload warnings #24

Open
ryanto opened this issue Feb 12, 2018 · 9 comments
Open

Payload warnings #24

ryanto opened this issue Feb 12, 2018 · 9 comments

Comments

@ryanto
Copy link
Member

ryanto commented Feb 12, 2018

warn.js:48 WARNING: You pushed a record of type 'clip' with a
relationship 'comments' configured as 'async: false'. You've included 
a link but no primary data, this may be an error in your payload.

With sync relationships the presence of a link throws an error. Let's filter these warnings out.

@samselikoff
Copy link
Contributor

👍 on filter the warning

@YoranBrondsema
Copy link

I don't use ember-data-storefront but I'm using similar patterns, i.e. async relationships everywhere but with links in the payload too. Therefore I am seeing a lot of these warnings. I filtered out these warnings by adding the following initializer to my app:

// app/initializers/warnings-to-ignore.js

import { registerWarnHandler } from '@ember/debug';

const IGNORED_WARNINGS = [
  'ds.store.push-link-for-sync-relationship'
];

export default {
  name: 'warnings-to-ignore',
  initialize() {
    registerWarnHandler(function(message, options, next) {
      if (!ignoreWarning(options)) {
        next(...arguments);
      }
    });
  }
};

function ignoreWarning(options) {
  return options && options.id && IGNORED_WARNINGS.includes(options.id);
}

This uses the API described in RFC 65.

I thought I'd share this snippet as it might help getting the functionality into the addon!

@ryanto
Copy link
Member Author

ryanto commented Apr 4, 2018

Awesome! That looks great.

I think this is a good first PR for anyone who wants to get involved w/ storefront!

@XaserAcheron
Copy link

Hmm, I ran into this a while back and went with the solution posted by @YoranBrondsema. Worked great, but, I just updated to Ember 3.7 and now the warnings are back. It doesn't look like the warn handler isn't even being called any more -- has anyone experienced this, by chance?

@nadnoslen
Copy link

Hey @XaserAcheron, I just leveraged successfully in my Ember-3.10 app. Is you import correct: @ember/debug?

@msmyers
Copy link

msmyers commented Sep 5, 2019

RE: everyone.

Hacking the solution via deleting links is a poor solution. Why can't ember-data just do additional ajax requests to resolve links?

Here's what a normal human would expect:

with User -> Posts, when I have async:false, when I query User, it should ajax to /users, and then discover links, then ajax to the links, AND ONLY THEN return to the initial caller.

Anything else is just lazily exposing the sausage-making.

Fetching User is an ajax call, which takes time, and is async. I don't care how many calls are shoved/packed into it, because I'm already async at that point. The goal/point of async:false is maintained, regardless of how many sausage-making underlying ajax calls that requires.

This is one of those reasons that React is better than Ember. They take no position, which makes ember-data's poor decision worse for developers to learn. It's easier to make it work when you make it yourself. It's harder to make it work when the framework you're expected to make work has warts.

Hiding warnings is not the same thing as fixing the problem. Hiding the warnings makes the code-that-works even harder to understand for someone else. Random people who "see Ember code" just scratch their head later.

YOU know why those warnings were hidden (and where they were hidden)... but who else?


Sure, the problem is with ember-data

ember-data-storefront already overrides many of the behaviors. The idea that "my first fetch should be good enough" is one of them that I would support investigating.

IF that's not possible (I honestly have no idea), then I recommend including the problem (and recommendation for solutions) it in the docs. I spent the entire day integrating ember-data-storefront in my app. I had to change my server to match the API-expectations of my client, and that's just evidence of a poor framework that can't handle standard JSON API spec.

Yes. I'm bitter. Thanks for listening to my rant. ember-data-storefront is the best solution to the zany ember-data than I've seen in a long time. Great work. I wish I were on the team.

Maybe I'm barking up the right tree. Just like my dog, sometimes barking is all I have... :)


I advocate for the principle of least surprise. Masking over warnings is not that.

@samselikoff
Copy link
Contributor

@msmyers I appreciate you sharing your thoughts – sounds like you had a rough day! Believe, I've been there and I understand how frustrating this stuff can be.

Since this is all fresh in your mind, I'd love to ask you a few questions:

  1. What led you installing Storefront to begin with? Was there a specific pain point or did you just want to start using the APIs?

  2. You said you had to change your server to match the API expectations of your client. Could you elaborate on this? Are you saying your use of Storefront forced you to change the API contract provided by your server?

As far as your recommendation to change Ember Data's async find behavior, I'm not sure I understand what you're proposing. Are you saying if you were to use one of Storefront's APIs (e.g. store.loadRecord('post') and that call returned a promise, that we shouldn't resolve that promise until all of the post's relationships were loaded? What if the caller didn't request any? What if there are 10 relationships on the post that all have links?

The real problem is the impedance mismatch between how many people use async: false models and what Ember Data means by async: false. Our approach has been to use Storefront as a playground for our ideal API, and an abstraction layer to paper over the parts of ED we find problematic. In our opinion using async: false models yields more predictable code, because the async parts are all triggered explicitly by the developer.

Did you try to use the warning suppression in this issue? Did you find it surprising? Or are you talking about some other aspect of Storefront?

Thanks again for the issue & I look forward to hearing your feedback!

@msmyers
Copy link

msmyers commented Sep 5, 2019

Thanks for the quick reply and opportunity to share my recent experiences.

I am starting a brand new project, using ember and sails, with the sails-ember-rest plugin.

With sails-ember-rest configuration:


  1. I was starting a project, looking for good fundamentals, and wanted to "do things updated/right" in this project.

I knew that I wanted to have 4 levels of one-to-many, and googled best-practices. I came across a blog entry that made a compelling argument/rationale for leveraging {async:false} relationships.

The selling point was the ability to crash if not preloaded, plus the ability to wire up computed properties. I built other projects in sails/ember, and wanted to avoid the pain of promise-aware templates.

  1. Yes, I had to change my server configuration from links to records. It is possible that I mis-interpreted that this was needed, but it seemed to be the only solution I was able to find.

  2. My concern was that I interpreted the warning to mean: "This record has stuff we can't resolve so we're going to assume it doesn't exist" - causing me to be concerned that when I fetched for children, the "caching of storefront" would return the cached 'empty' array.


Now that I think about it, I'm pretty happy with how it snapped into my project. I don't have any requested changes. I believe the problem was potentially in my understanding and expectations of the library.

@samselikoff
Copy link
Contributor

Gotcha.

I'm guessing the blog post you're referring to is this one of ours: https://embermap.com/notes/83-the-case-against-async-relationships

There's indeed some confusing pieces here because of the overlap with Ember Data's APIs and Storefront, which really can be thought of an opinionated way to use Ember Data.

I think there's a place in the guides under the [Avoiding async relationships](Avoiding async relationships) section to explain this warning, that it's coming from Ember Data (not Storefront), and why it doesn't affect Storefront's functionality or cache.

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

No branches or pull requests

7 participants
@ryanto @msmyers @YoranBrondsema @samselikoff @XaserAcheron @nadnoslen and others