search

senecajs / seneca-entity

Entity plugin for seneca
MIT License
13 stars 15 forks source link

Move opts extend to preload #25

Closed nfantone closed 8 years ago

nfantone commented 8 years ago

As mentioned in #24, this fixes options overriding and allows users to disable mem_store.

preload function gets invoked before the default export, thus ignoring options received in function entity.

Also, could I vote in favor of disabling mem_store by default? I don't know how many people actively use the plugin, but I can't tell you how many times this has been a cause of bug-searching and head-scratching for hours. Your whole microservice doesn't work as expected, but looks like it does. Until you remember. Your transport layer isn't picking your AMQP config up: it's using the unfathomable MemStore! All due to a somewhat hidden property you didn't even remember existed (mem_store: false). So you say to yourself: "Crap. This again. Next time, I'll remember about it". And maybe you do, but then the next dev comes along and it's crying wolf all over again.

If this sound a bit like a rant, it's because it probably is. I had to write a very silly wrapper around Seneca whose main purpose is to configure an AMQP transport and disable mem_store, just to get around this behaviour. And I thought I was covered with that... until I upgraded seneca-entity to latest, which re-enabled mem_store.

I get the idea behind including the store by default. It's nice when things work out of the box. But it's nicer when you set things up to use a transport and your whole plugin ecosystem uses that transport. This is one of those things that "reads nice" when you are taking a look at an online doc, but it practice, it doesn't really work. You should't rely on some magical store (bar demoware) - but if you do want to rely on it, you could always turn it on.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.06%) to 80.057% when pulling e076da05f7a4cf74bfac97587753018bc2b614d2 on nfantone:bug/disable-memstore into ab05758ed9f113f266889dbcf1bdf0f9c88f2f21 on senecajs:master.

mihaidma commented 8 years ago

Thank you, seems like a very important finding.

mcdonnelldean commented 8 years ago

@nfantone Great catch, thanks for this.

You are correct, and that will be the plan for 4.0 later in the year. We originally took it out, which caused huge reported breakages so we added it back in for now (since by default seneca did also). But we missed this.