search

unlight / sigh-bify

sigh-plugin for browserify.
0 stars 0 forks source link

Review #1

Closed unlight closed 8 years ago

unlight commented 8 years ago

@ohjames Can you review this, please? src/index

insidewhy commented 8 years ago

Looks good, a few general comments.

  1. Out of interest, why not sigh-browserify instead of sigh-bify?
  2. Why not let instead of var? (if you look at the sigh code, it doesn't use block scoping, this is due to code generations with older versions of babel though and will be changed)
insidewhy commented 8 years ago

Hm it won't let me comment on the code as it's not a PR.

I'd also try to avoid Bacon.Bus, I think Bacon might already have some kinda fromEvent helper.

insidewhy commented 8 years ago

One other thing, wondered if you'd looked at jspm? Really glad you're helping to support browserify since so many people are using it, I think jspm+sigh/gulp is a technically superior option to browserify.

unlight commented 8 years ago

@ohjames Thank you for review.

why not sigh-browserify instead of sigh-bify

I decided to save name sigh-browserify for you (official guys) and pick something other (to prevent collision sigh-browserify vs sigh-browserify (official). I did not want to take name sigh-browserify because of concern that it will not be implemented properly (in sigh-way). FRP with Bacon.js is new to me. By the way, are you planning to stick Bacon.js? Do you have any intentions to switch to kefir.js which has a better performance and less usage of memory, and better documentation?

The argument b seems wrong, sigh only passes opts?

Main argument is b (browserify instance), but in some cases it need pass additional (optional) parameters are not related to browserify.

I think jspm+sigh/gulp is a technically superior option to browserify.

Maybe. But for now jspm has lack of plugins and maybe bigger overhead.

unlight commented 8 years ago

One more thing, why stream looks as single event value of array of files. Why not event = file?

insidewhy commented 8 years ago

Sorry for the delayed response, if the payload was a single event... what about 1:n plugins? Then they have to send multiple events, but if a later stream operation wants to merge events to avoid redundant work? Then it'll have to debounce (introducing a non-busy wait). With an array of files per event you don't have these issues.