jagi / meteor-astronomy

Model layer for Meteor
https://atmospherejs.com/jagi/astronomy
MIT License
604 stars 67 forks source link

Does not play well with meteorhacks:fast-render or kadira:flow-router-ssr (was Warnings about non-existent fields I didn't create) #136

Closed ndarilek closed 9 years ago

ndarilek commented 9 years ago

Seeing lots of this in my console:

Trying to set a value of the "_modifiers" field that does not exist in the "User" class
jagi_as...6ba563a (line 80)
warn:
Trying to set a value of the "_original" field that does not exist in the "User" class
jagi_as...6ba563a (line 80)
warn:
Trying to set a value of the "_isNew" field that does not exist in the "User" class
jagi_as...6ba563a (line 80)

Seeing this warning in all my schemas.

Seems like maybe jagi:reactive-map is adding these keys to all my objects for some reason, and because they aren't in my schema, they're throwing warnings. You can see this behavior in the test repository I linked yesterday. These fields are not in the database, but do appear now in find or findOne calls. Not sure if that is the package causing this, but a quick grep through .meteor/local for _isNew seems to point to that. Maybe it's a dependency.

Thanks.

lukejagodzinski commented 9 years ago

Please read these two sections http://astronomy.jagi.io/#publications and http://astronomy.jagi.io/#warnings. After reading it you should know how to resolve your problem. If not, let me know.

ndarilek commented 9 years ago

I don't know that I'm doing any of this, though. The test app I posted yesterday at:

https://github.com/ndarilek/meteor-astronomy-event-issue

is actually pretty minimal. It literally just creates a user using Meteor's own functions, but I still see these fields which I never added. Leaving aside the issue that afterInsert doesn't work, you see lots of these warnings about _isNew and related fields which I never created.

Additionally, I don't know that I added jagi:reactive-map. Not sure if it is the reason behind these fields, but a quick grep through .meteor/local points to it as a possibility.

I guess I can turn off warnings if needed, but it seems a bit odd that a package on which Astronomy depends might be causing them. If that is indeed the case, then why not just have Astronomy itself turn off its own warnings? Isn't warning about variables created by a dependent package just making the warnings system emit unnecessary noise and more likely to be ignored/turned off?

ndarilek commented 9 years ago

Sorry, don't mean for the last comment to sound hostile or negative, I'm just supremely confused by the sudden appearance of these new fields, and since I want schema validation, I'd rather not turn off warnings if I can avoid it.

Thanks for the quick responses to all of these issues.

lukejagodzinski commented 9 years ago

The jagi:reactive-map is not a source of this problem. It's most common to appear when you are using reactive publications. Are you using any package for that?

In this minimal example you haven't defined any fields so why are you surprised with the warning? If there is no username field then it will show warning that it's trying to set the username field that does not exist. Please read documentation carefully first. In this section in the documentation you have an example of Astronomy class for the Meteor.users collection.

ndarilek commented 9 years ago

I've found the issue, or at least, I've found a pointer to the cause.

The issue goes away when I remove meteorhacks:fast-render.

Please note, I am actively and extensively reading your docs, but when they ask if I'm doing any kind of crazy reactive publishing, and I know that I'm not, asking me to read more docs won't help. :) They've also got some accessibility issues when I try reading them with my screen reader, so I've had to check out the meteor-astronomy-docs repository and open individual markdown pages in my text editor to read them. Looks like you're doing something with scrolling that makes NVDA (my screen reader) unhappy when I read the page by headings. Focus bounces pretty aggressively into previous sections, and the only way I've found to read your documentation is looking at the source itself. Unfortunately it's a bit tough tracking down individual feature docs but I think I've mostly got a handle on it now.

So it seems that fast-render is doing something Astronomy doesn't like and causing these errors to appear. Removing it from my project for the time being. I think there were also some issues with kadira:flow-router-ssr, but switching to kadira:flow-router made those errors vanish. I'd investigate further but for the moment I just want to get this refactor complete. :)

Thanks.

lukejagodzinski commented 9 years ago

Ok good to hear. I haven't been using this packages and I don't know why they are causing these warnings.

Yes I know about problems with reading documentation on mobile devices. I will have to split it into smaller sections and improve scroll position detection.

You have to forgive me, that I'm sending people to the documentation. But I'm getting many similar questions over and over again. And most of the time, the reason for an error is on the side of developer :). Very often, people don't even read a documentation, or they don't understand how Meteor works. If someone don't know how Meteor works, they will not understand how Astronomy works.

As I'm writing it, I think that people lack of ability to do debugging. It's crucial, especially when programming with Meteor. Maybe someone should create some good tutorial about debugging Meteor :).

ndarilek commented 9 years ago

Gotcha, that's fair re: reading the docs.

Might be good to retitle this issue. I think meteorhacks:fast-render+kadira:flow-router is supposed to be add-and-forget with no app-side code changes, and me adding fast-render triggered its built-in flow-router integration. But obviously that does something Astronomy isn't liking. Not sure if there's any way Astronomy might detect this and work around it, but I imagine others will hit it at some point.

A debugging tutorial might be interesting. In my case, the extent of my debugging was looking at .meteor/packages, wondering what might be causing a bad interaction, and resolving to meteor remove until I found it. Fortunately I guessed right on my first try, would've been a long day if I hadn't. :)

Thanks.

lukejagodzinski commented 9 years ago

The problem with warnings is actually quite common and I'm thinking about solving it globally. It's mostly caused by an assumption of the package author that a document coming from a collection is a plain JavaScript object and it's not true if it goes about Astronomy.

It it's only warning, then it's not a big deal. Some packages does not work at all, and you have to disable transformation of documents. I'm going to implement a workaround for that.

lukejagodzinski commented 9 years ago

OK I've added a possibility to turn off transformation for the server. I've described it here: http://astronomy.jagi.io/#custom-transform