express-bem / bh

bh.js engine for express-bem
https://github.com/express-bem/express-bem
MIT License
1 stars 2 forks source link

Improve data support #2

Closed remnev closed 9 years ago

remnev commented 9 years ago

Hi, Alex!

I use pure Express, which exposes locals fields of app and res to collect data as native approach. I'd like to pass this data to templates. Please, look at what I propose in code. It's the blocker thing, that stops me to use the module. I don't want to do a fork with these changes. I want to improve this module instead.

BTW, I added code coverage to see how well tests work. I wasn't able to write test for check the template requiring. I'll be glad if you'll help me with it at the future.

Ok, I look forward to your comments.

qfox commented 9 years ago

At first, thanks for contribution ;-)

It's a nice idea, but passing options as data looks like a hardcode. I've suggested a little compromise to fit your needs and don't break anything else.

Also I've left some nits about codestyle (which is mutable for now). All other changes is pretty nice and I'll land them for sure ;-)

Thanks again.

upd

I wasn't able to write test for check the template requiring. I'll be glad if you'll help me with it at the future.

I'm not sure we really need to test that because it should be guaranteed by npm. But there are some solutions for that if you insist.

btw good work!

remnev commented 9 years ago

But there are some solutions for that if you insist.

I like when code is fully covered by tests. So, I'd like to cover this branch. But it may be another issue. BTW, we need to improve this peace of code:

// check cache options
if (opts.force || options.forceExec || options.forceLoad) {
    dropRequireCache(require, name);
}

options.forceLoad is not covered by tests. And I'm not sure that we need all of these options. May be it makes reason to have only one? It may be another issue too.

Please, see my new commits. I'm going to update documentation and will be ready to merge it :)

qfox commented 9 years ago

Coverage is about covering ;-)

We can cover force* easily, it's not a problem. I've left these just to be consistent with earlier version of express-bem, it's ok if we change it.

LGTM btw. If you ready to document these — it'd be great!

remnev commented 9 years ago

Alex, please, see the commit about documentation. I've tried to describe the usage with more details. And the example works fine with pure express.

We can cover force* easily, it's not a problem.

But how? :)

remnev commented 9 years ago

Can we merge it? I feel hard needs.

qfox commented 9 years ago

Hey! ;-)

qfox commented 9 years ago

I'm repushed my work to master (sorry if you already used it) and added coveralls.

Landed this into v1.1.0. Thanks!

remnev commented 9 years ago

Ok, I hope we will do any changes via pull requests in next times. And only after successful approving by somebody of us :) If you don't mind.